ResolveMerger: don't try needlessly to delete directories

Don't try to delete folders if the merger chooses THEIRS, but all of
BASE, OURS, and THEIRS contain the folder.

Add a test for rebase with auto-stash and subdirectories that
verifies this case. The needless directory deletion and reporting
such directories in getModifiedFiles() was the root cause of bug
536880.

Note even with this fix, bug 536880 will not be fixed in all cases
yet. There may still be cases where the set of modified files ends
up containing directories. This will be dealt with in EGit where
this set is used. (See https://git.eclipse.org/r/#/c/126242/ .)

Bug: 536880
Change-Id: I62b4571a1c1d4415934a6cb4270e0c8036deb2e9
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

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 96e7091ae15959252219295d6009a60d302a1315..621e061c7912b31e9bdf59a9abf2e9fe9953e80d 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java
@@ -57,9 +57,12 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 
 import org.eclipse.jgit.api.MergeResult.MergeStatus;
 import org.eclipse.jgit.api.RebaseCommand.InteractiveHandler;
@@ -75,6 +78,7 @@ import org.eclipse.jgit.errors.AmbiguousObjectException;
 import org.eclipse.jgit.errors.IllegalTodoFileModification;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.events.ListenerHandle;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.lib.AbbreviatedObjectId;
 import org.eclipse.jgit.lib.ConfigConstants;
@@ -1980,6 +1984,62 @@ public class RebaseCommandTest extends RepositoryTestCase {
                assertEquals(RepositoryState.SAFE, db.getRepositoryState());
        }
 
+       @Test
+       public void testRebaseWithAutoStashAndSubdirs() throws Exception {
+               // create file0, add and commit
+               db.getConfig().setBoolean(ConfigConstants.CONFIG_REBASE_SECTION, null,
+                               ConfigConstants.CONFIG_KEY_AUTOSTASH, true);
+               writeTrashFile("sub/file0", "file0");
+               git.add().addFilepattern("sub/file0").call();
+               git.commit().setMessage("commit0").call();
+               // create file1, add and commit
+               writeTrashFile(FILE1, "file1");
+               git.add().addFilepattern(FILE1).call();
+               RevCommit commit = git.commit().setMessage("commit1").call();
+
+               // create topic branch and checkout / create file2, add and commit
+               createBranch(commit, "refs/heads/topic");
+               checkoutBranch("refs/heads/topic");
+               writeTrashFile("file2", "file2");
+               git.add().addFilepattern("file2").call();
+               git.commit().setMessage("commit2").call();
+
+               // checkout master branch / modify file1, add and commit
+               checkoutBranch("refs/heads/master");
+               writeTrashFile(FILE1, "modified file1");
+               git.add().addFilepattern(FILE1).call();
+               git.commit().setMessage("commit3").call();
+
+               // checkout topic branch / modify file0
+               checkoutBranch("refs/heads/topic");
+               writeTrashFile("sub/file0", "unstaged modified file0");
+
+               Set<String> modifiedFiles = new HashSet<>();
+               ListenerHandle handle = db.getListenerList()
+                               .addWorkingTreeModifiedListener(
+                                               event -> modifiedFiles.addAll(event.getModified()));
+               try {
+                       // rebase
+                       assertEquals(Status.OK, git.rebase()
+                                       .setUpstream("refs/heads/master").call().getStatus());
+               } finally {
+                       handle.remove();
+               }
+               checkFile(new File(new File(db.getWorkTree(), "sub"), "file0"),
+                               "unstaged modified file0");
+               checkFile(new File(db.getWorkTree(), FILE1), "modified file1");
+               checkFile(new File(db.getWorkTree(), "file2"), "file2");
+               assertEquals(
+                               "[file1, mode:100644, content:modified file1]"
+                                               + "[file2, mode:100644, content:file2]"
+                                               + "[sub/file0, mode:100644, content:file0]",
+                               indexState(CONTENT));
+               assertEquals(RepositoryState.SAFE, db.getRepositoryState());
+               List<String> modified = new ArrayList<>(modifiedFiles);
+               Collections.sort(modified);
+               assertEquals("[file1, sub/file0]", modified.toString());
+       }
+
        @Test
        public void testRebaseWithAutoStashConflictOnApply() throws Exception {
                // create file0, add and commit

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java
index ad3ab7fbdfb38745c2b0fac4861ffa0534cc7567..fa6c9609f5f867a9707f122a2da2187c25f0c7a9 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/StashApplyCommandTest.java
@@ -234,7 +234,7 @@ public class StashApplyCommandTest extends RepositoryTestCase {
                ObjectId unstashed = git.stashApply().call();
                assertEquals(stashed, unstashed);
                assertEquals("content2", read(subfolderFile));
-               recorder.assertEvent(new String[] { "d1/d2/f.txt", "d1/d2", "d1" },
+               recorder.assertEvent(new String[] { "d1/d2/f.txt" },
                                ChangeRecorder.EMPTY);
 
                Status status = git.status().call();

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
index da6a3da67d9c48675e6298790a3bc458bae7838b..3042fdd3f4d2f4badf04e3864972bf20886b768c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
@@ -632,12 +632,16 @@ public class ResolveMerger extends ThreeWayMerger {
                                return true;
                        } else {
                                // we want THEIRS ... but THEIRS contains a folder or the
-                               // deletion of the path. Delete what's in the workingtree (the
-                               // workingtree is clean) but do not complain if the file is
-                               // already deleted locally. This complements the test in
-                               // isWorktreeDirty() for the same case.
-                               if (tw.getTreeCount() > T_FILE && tw.getRawMode(T_FILE) == 0)
+                               // deletion of the path. Delete what's in the working tree,
+                               // which we know to be clean.
+                               if (tw.getTreeCount() > T_FILE && tw.getRawMode(T_FILE) == 0) {
+                                       // Not present in working tree, so nothing to delete
                                        return true;
+                               }
+                               if (modeT != 0 && modeT == modeB) {
+                                       // Base, ours, and theirs all contain a folder: don't delete
+                                       return true;
+                               }
                                toBeDeleted.add(tw.getPathString());
                                return true;
                        }