From 51c20b27acdef57dcb1d79ac995cd49f2e9c2f91 Mon Sep 17 00:00:00 2001 From: Robin Stocker Date: Thu, 16 Aug 2012 16:06:58 +0200 Subject: [PATCH] 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 --- .../eclipse/jgit/api/RebaseCommandTest.java | 32 +++++++++++++++++++ .../jgit/lib/DirCacheCheckoutTest.java | 15 ++++----- .../jgit/dircache/DirCacheCheckout.java | 30 +++++++++-------- 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 dbad322be..81730b93a 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 6c6c911f2..23647f06d 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 headEntries, HashMap mergeEntries, HashMap 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 d43fef847..978099365 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 { /** * 
-			 * 		    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
 			 *
*/ @@ -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) { /**