DirCacheCheckout: Fix handling of files not in index

When a file is not in the index and neither contents nor mode differ
between "head" and "merge", the index state should be kept. If they
differ, a checkout conflict should occur. This is described in Git's
git-read-tree.txt.

JGit used to replace the index state with "merge" in both of the above
cases.

A confusing effect of this was that when one removed a file and then did
a rebase, the file silently reappeared again.

The changes to dir/file conflict handling are a consequence of this
change, as the index handling change made tests in DirCacheCheckoutTest
break. I compared these cases to C Git and the new behavior there also
matches what C Git does.

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

3 files changed, 54 insertions, 23 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
index dbad322be2..81730b93aa 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
@@ -1368,6 +1368,38 @@ public class RebaseCommandTest extends RepositoryTestCase {
 		assertEquals(RepositoryState.SAFE, db.getRepositoryState());
 	}
 
+	@Test
+	public void testRebaseWithUncommittedDelete() throws Exception {
+		// create file0 + file1, add and commit
+		File file0 = writeTrashFile("file0", "file0");
+		writeTrashFile(FILE1, "file1");
+		git.add().addFilepattern("file0").addFilepattern(FILE1).call();
+		RevCommit commit = git.commit().setMessage("commit1").call();
+
+		// create topic branch
+		createBranch(commit, "refs/heads/topic");
+
+		// still on master / modify file1, add and commit
+		writeTrashFile(FILE1, "modified file1");
+		git.add().addFilepattern(FILE1).call();
+		git.commit().setMessage("commit2").call();
+
+		// checkout topic branch / delete file0 and add to index
+		checkoutBranch("refs/heads/topic");
+		git.rm().addFilepattern("file0").call();
+		// do not commit
+
+		// rebase
+		RebaseResult result = git.rebase().setUpstream("refs/heads/master")
+				.call();
+		assertEquals(Status.FAST_FORWARD, result.getStatus());
+		assertFalse("File should still be deleted", file0.exists());
+		// index should only have updated file1
+		assertEquals("[file1, mode:100644, content:modified file1]",
+				indexState(CONTENT));
+		assertEquals(RepositoryState.SAFE, db.getRepositoryState());
+	}
+
 	private int countPicks() throws IOException {
 		int count = 0;
 		File todoFile = getTodoFile();
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
index 6c6c911f23..23647f06d7 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
@@ -261,11 +261,10 @@ public class DirCacheCheckoutTest extends RepositoryTestCase {
 
 		merge = buildTree(mkmap("foo", "a"));
 		tw = TreeWalk.forPath(db, "foo", merge);
-		ObjectId anotherId = tw.getObjectId(0);
 
 		prescanTwoTrees(head, merge);
 
-		assertEquals(anotherId, getUpdated().get("foo"));
+		assertConflict("foo");
 	}
 
 	void setupCase(HashMap<String, String> headEntries, HashMap<String, String> mergeEntries, HashMap<String, String> indexEntries) throws IOException {
@@ -464,8 +463,8 @@ public class DirCacheCheckoutTest extends RepositoryTestCase {
 	 *     ------------------------------------------------------------------
 	 *1    D        D       F       Y         N       Y       N           Update
 	 *2    D        D       F       N         N       Y       N           Conflict
-	 *3    D        F       D                 Y       N       N           Update
-	 *4    D        F       D                 N       N       N           Update
+	 *3    D        F       D                 Y       N       N           Keep
+	 *4    D        F       D                 N       N       N           Conflict
 	 *5    D        F       F       Y         N       N       Y           Keep
 	 *6    D        F       F       N         N       N       Y           Keep
 	 *7    F        D       F       Y         Y       N       N           Update
@@ -522,18 +521,16 @@ public class DirCacheCheckoutTest extends RepositoryTestCase {
 
 	@Test
 	public void testDirectoryFileConflicts_3() throws Exception {
-		// 3 - the first to break!
+		// 3
 		doit(mk("DF/DF"), mk("DF/DF"), mk("DF"));
-		assertUpdated("DF/DF");
-		assertRemoved("DF");
+		assertNoConflicts();
 	}
 
 	@Test
 	public void testDirectoryFileConflicts_4() throws Exception {
 		// 4 (basically same as 3, just with H and M different)
 		doit(mk("DF/DF"), mkmap("DF/DF", "foo"), mk("DF"));
-		assertUpdated("DF/DF");
-		assertRemoved("DF");
+		assertConflict("DF/DF");
 
 	}
 
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
index d43fef8477..9780993659 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
@@ -552,8 +552,8 @@ public class DirCacheCheckout {
 		 *      ------------------------------------------------------------------
 		 * 1    D        D       F       Y         N       Y       N           Update
 		 * 2    D        D       F       N         N       Y       N           Conflict
-		 * 3    D        F       D                 Y       N       N           Update
-		 * 4    D        F       D                 N       N       N           Update
+		 * 3    D        F       D                 Y       N       N           Keep
+		 * 4    D        F       D                 N       N       N           Conflict
 		 * 5    D        F       F       Y         N       N       Y           Keep
 		 * 6    D        F       F       N         N       N       Y           Keep
 		 * 7    F        D       F       Y         Y       N       N           Update
@@ -615,10 +615,7 @@ public class DirCacheCheckout {
 
 				break;
 			case 0xDFD: // 3 4
-				// CAUTION: I put it into removed instead of updated, because
-				// that's what our tests expect
-				// updated.put(name, mId);
-				remove(name);
+				keep(dce);
 				break;
 			case 0xF0D: // 18
 				remove(name);
@@ -703,12 +700,13 @@ public class DirCacheCheckout {
 
 			/**
 			 * <pre>
-			 * 		    I (index)                H        M        Result
-			 * 	        -------------------------------------------------------
-			 * 	        0 nothing             nothing  nothing  (does not happen)
-			 * 	        1 nothing             nothing  exists   use M
-			 * 	        2 nothing             exists   nothing  remove path from index
-			 * 	        3 nothing             exists   exists   use M
+			 * 	          I (index)     H        M     H==M  Result
+			 * 	        -------------------------------------------
+			 * 	        0 nothing    nothing  nothing        (does not happen)
+			 * 	        1 nothing    nothing  exists         use M
+			 * 	        2 nothing    exists   nothing        remove path from index
+			 * 	        3 nothing    exists   exists   yes   keep index
+			 * 	          nothing    exists   exists   no    fail
 			 * </pre>
 			 */
 
@@ -716,8 +714,12 @@ public class DirCacheCheckout {
 				update(name, mId, mMode); // 1
 			else if (m == null)
 				remove(name); // 2
-			else
-				update(name, mId, mMode); // 3
+			else { // 3
+				if (equalIdAndMode(hId, hMode, mId, mMode))
+					keep(dce);
+				else
+					conflict(name, dce, h, m);
+			}
 		} else {
 			if (h == null) {
 				/**