Fix DirCacheCheckout to return CheckoutConflictException

Problem occurs when the checkout wants to create a file 'd/f' but
the workingtree contains a dirty file 'd'. In order to create d/f the
file 'd' would have to be deleted and since the file is dirty that
content would be lost. This should lead to a CheckoutConflictException
for d/f when failOnConflict was set to true.

This fix also changes jgit checkout semantics to be more like native
gits checkout semantics. If during a checkout jgit wants to delete a
folder but finds that the working tree contains a dirty file at this
path then JGit will now throw an exception instead of silently keeping
the dirty file. Like in this example:

	git init
	touch b
	git add b
	git commit -m addB
	mkdir a
	touch a/c
	git add a/c
	git commit -m addAC
	rm -fr a
	touch a
	git checkout HEAD~

Change-Id: I9089123179e09dd565285d50b0caa308d290cccd
Signed-off-by: Rüdiger Herrmann <ruediger.herrmann@gmx.de>
Also-by: Rüdiger Herrmann <ruediger.herrmann@gmx.de>

diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java
index e690ad696427ee610b717938f1a4e107b809b6a9..3651542fc65e148b8dc22a5d5e0d81383a142497 100644 (file)
--- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java
+++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/CheckoutTest.java
@@ -47,6 +47,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.File;
 import java.nio.file.Files;
@@ -236,8 +237,8 @@ public class CheckoutTest extends CLIRepositoryTestCase {
         * <li>Checkout branch '1'
         * </ol>
         * <p>
-        * The working tree should contain 'a' with FileMode.REGULAR_FILE after the
-        * checkout.
+        * The checkout has to delete folder but the workingtree contains a dirty
+        * file at this path. The checkout should fail like in native git.
         *
         * @throws Exception
         */
@@ -266,11 +267,15 @@ public class CheckoutTest extends CLIRepositoryTestCase {
                                        db.getFS());
                        assertEquals(FileMode.REGULAR_FILE, entry.getMode());
 
-                       git.checkout().setName(branch_1.getName()).call();
-
-                       entry = new FileTreeIterator.FileEntry(new File(db.getWorkTree(), "a"),
-                                       db.getFS());
-                       assertEquals(FileMode.REGULAR_FILE, entry.getMode());
+                       try {
+                               git.checkout().setName(branch_1.getName()).call();
+                               fail("Don't get the expected conflict");
+                       } catch (CheckoutConflictException e) {
+                               assertEquals("[a]", e.getConflictingPaths().toString());
+                               entry = new FileTreeIterator.FileEntry(
+                                               new File(db.getWorkTree(), "a"), db.getFS());
+                               assertEquals(FileMode.REGULAR_FILE, entry.getMode());
+                       }
                }
        }
 

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 fbe7dd04178945c2750c63ddc05eb099e738bf89..bb553a444e50db79fb97f4089112e10770336243 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
@@ -1614,6 +1614,64 @@ public class DirCacheCheckoutTest extends RepositoryTestCase {
                assertNotNull(git.checkout().setName(Constants.MASTER).call());
        }
 
+       @Test(expected = CheckoutConflictException.class)
+       public void testFolderFileConflict() throws Exception {
+               RevCommit headCommit = commitFile("f/a", "initial content", "master");
+               RevCommit checkoutCommit = commitFile("f/a", "side content", "side");
+               FileUtils.delete(new File(db.getWorkTree(), "f"), FileUtils.RECURSIVE);
+               writeTrashFile("f", "file instead of folder");
+               new DirCacheCheckout(db, headCommit.getTree(), db.lockDirCache(),
+                               checkoutCommit.getTree()).checkout();
+       }
+
+       @Test
+       public void testMultipleContentConflicts() throws Exception {
+               commitFile("a", "initial content", "master");
+               RevCommit headCommit = commitFile("b", "initial content", "master");
+               commitFile("a", "side content", "side");
+               RevCommit checkoutCommit = commitFile("b", "side content", "side");
+               writeTrashFile("a", "changed content");
+               writeTrashFile("b", "changed content");
+
+               try {
+                       new DirCacheCheckout(db, headCommit.getTree(), db.lockDirCache(),
+                                       checkoutCommit.getTree()).checkout();
+                       fail();
+               } catch (CheckoutConflictException expected) {
+                       assertEquals(2, expected.getConflictingFiles().length);
+                       assertTrue(Arrays.asList(expected.getConflictingFiles())
+                                       .contains("a"));
+                       assertTrue(Arrays.asList(expected.getConflictingFiles())
+                                       .contains("b"));
+                       assertEquals("changed content", read("a"));
+                       assertEquals("changed content", read("b"));
+               }
+       }
+
+       @Test
+       public void testFolderFileAndContentConflicts() throws Exception {
+               RevCommit headCommit = commitFile("f/a", "initial content", "master");
+               commitFile("b", "side content", "side");
+               RevCommit checkoutCommit = commitFile("f/a", "side content", "side");
+               FileUtils.delete(new File(db.getWorkTree(), "f"), FileUtils.RECURSIVE);
+               writeTrashFile("f", "file instead of a folder");
+               writeTrashFile("b", "changed content");
+
+               try {
+                       new DirCacheCheckout(db, headCommit.getTree(), db.lockDirCache(),
+                                       checkoutCommit.getTree()).checkout();
+                       fail();
+               } catch (CheckoutConflictException expected) {
+                       assertEquals(2, expected.getConflictingFiles().length);
+                       assertTrue(Arrays.asList(expected.getConflictingFiles())
+                                       .contains("b"));
+                       assertTrue(Arrays.asList(expected.getConflictingFiles())
+                                       .contains("f"));
+                       assertEquals("file instead of a folder", read("f"));
+                       assertEquals("changed content", read("b"));
+               }
+       }
+
        public void assertWorkDir(Map<String, String> i)
                        throws CorruptObjectException,
                        IOException {

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 12ceb74abb12c210a4a3d401912891c19c9dde49..ab61c593781ac2957a7379eeb7112ce6a0c640d6 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
@@ -718,10 +718,23 @@ public class DirCacheCheckout {
                        return;
                }
 
-               // if we have no file at all then there is nothing to do
-               if ((ffMask & 0x222) == 0
-                               && (f == null || FileMode.TREE.equals(f.getEntryFileMode())))
-                       return;
+               if ((ffMask & 0x222) == 0) {
+                       // HEAD, MERGE and index don't contain a file (e.g. all contain a
+                       // folder)
+                       if (f == null || FileMode.TREE.equals(f.getEntryFileMode())) {
+                               // the workingtree entry doesn't exist or also contains a folder
+                               // -> no problem
+                               return;
+                       } else {
+                               // the workingtree entry exists and is not a folder
+                               if (!idEqual(h, m)) {
+                                       // Because HEAD and MERGE differ we will try to update the
+                                       // workingtree with a folder -> return a conflict
+                                       conflict(name, null, null, null);
+                               }
+                               return;
+                       }
+               }
 
                if ((ffMask == 0x00F) && f != null && FileMode.TREE.equals(f.getEntryFileMode())) {
                        // File/Directory conflict case #20
@@ -1004,6 +1017,17 @@ public class DirCacheCheckout {
                }
        }
 
+       private static boolean idEqual(AbstractTreeIterator a,
+                       AbstractTreeIterator b) {
+               if (a == b) {
+                       return true;
+               }
+               if (a == null || b == null) {
+                       return false;
+               }
+               return a.getEntryObjectId().equals(b.getEntryObjectId());
+       }
+
        /**
         * A conflict is detected - add the three different stages to the index
         * @param path the path of the conflicting entry