From 028434e4f58bfc154da2f56a68e7aefc220bb359 Mon Sep 17 00:00:00 2001 From: Robin Stocker Date: Tue, 12 Jun 2012 18:12:14 +0200 Subject: [PATCH 1/2] Don't return success on failing paths in ResolveMerger ResolveMerger#mergeImpl() was only returning false (= failed) when there were unmerged paths. In the case when there were only failing paths, it returned true. Because MergeCommand looks at the return value for determining if the merge failed, it would fall into the successful case there, where it should instead return a MergeResult with MergeStatus.FAILED. This change adds a test case for this and makes the ResolveMerger return false when there are failing paths. This was discovered while working on fixing bug 354099 and is needed for its test case. Bug: 354099 Change-Id: I499f518f6289ef93e017db924b2aa857f2154707 Signed-off-by: Robin Stocker --- .../eclipse/jgit/merge/ResolveMergerTest.java | 98 +++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 2 +- 2 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java new file mode 100644 index 000000000..4cb089602 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java @@ -0,0 +1,98 @@ +/* + * Copyright (C) 2012, Robin Stocker + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.eclipse.jgit.merge; + +import static org.junit.Assert.assertFalse; + +import java.io.File; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.lib.RepositoryTestCase; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.treewalk.FileTreeIterator; +import org.eclipse.jgit.util.FileUtils; +import org.junit.Test; + +public class ResolveMergerTest extends RepositoryTestCase { + + @Test + public void failingPathsShouldNotResultInOKReturnValue() throws Exception { + File folder1 = new File(db.getWorkTree(), "folder1"); + FileUtils.mkdir(folder1); + File file = new File(folder1, "file1.txt"); + write(file, "folder1--file1.txt"); + file = new File(folder1, "file2.txt"); + write(file, "folder1--file2.txt"); + + Git git = new Git(db); + git.add().addFilepattern(folder1.getName()).call(); + RevCommit base = git.commit().setMessage("adding folder").call(); + + recursiveDelete(folder1); + git.rm().addFilepattern("folder1/file1.txt") + .addFilepattern("folder1/file2.txt").call(); + RevCommit other = git.commit() + .setMessage("removing folders on 'other'").call(); + + git.checkout().setName(base.name()).call(); + + file = new File(db.getWorkTree(), "unrelated.txt"); + write(file, "unrelated"); + + git.add().addFilepattern("unrelated").call(); + RevCommit head = git.commit().setMessage("Adding another file").call(); + + // Untracked file to cause failing path for delete() of folder1 + file = new File(folder1, "file3.txt"); + write(file, "folder1--file3.txt"); + + ResolveMerger merger = new ResolveMerger(db, false); + merger.setCommitNames(new String[] { "BASE", "HEAD", "other" }); + merger.setWorkingTreeIterator(new FileTreeIterator(db)); + boolean ok = merger.merge(head.getId(), other.getId()); + + assertFalse(merger.getFailingPaths().isEmpty()); + assertFalse(ok); + } + +} 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 7b1b36c94..6130cc72c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -221,7 +221,7 @@ protected boolean mergeImpl() throws IOException { builder = null; } - if (getUnmergedPaths().isEmpty()) { + if (getUnmergedPaths().isEmpty() && !failed()) { resultTree = dircache.writeTree(getObjectInserter()); return true; } else { From e623db0f876d95c9faae7ca089cb11c0bc2e6a7c Mon Sep 17 00:00:00 2001 From: Robin Stocker Date: Tue, 12 Jun 2012 19:03:52 +0200 Subject: [PATCH 2/2] Fix order of deletion for files/dirs in ResolveMerger Before, the paths to delete were stored in a HashMap, which doesn't have a particular order. So when e.g. both the file "a/b" and the directory "a" were to be deleted, it would sometimes try to delete "a" first. This resulted in a failed path because File#delete() fails when a directory isn't empty. With this change, an ArrayList is used for storing the paths to delete. The list contains the paths in a top-down order, as defined by the order of processEntry. When the files are deleted, the list is iterated in reverse, ensuring that all files of a directory are deleted before the directory itself. Bug: 354099 Change-Id: I6b2ce96b3932ca84ecdfbeab457ce823c95433fb Signed-off-by: Robin Stocker --- .../eclipse/jgit/api/MergeCommandTest.java | 45 +++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 25 +++++++---- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java index c6875b483..9effe6022 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/MergeCommandTest.java @@ -1037,6 +1037,51 @@ public void testMergeRemovingFolders() throws Exception { assertFalse(folder2.exists()); } + @Test + public void testMergeRemovingFoldersWithoutFastForward() throws Exception { + File folder1 = new File(db.getWorkTree(), "folder1"); + File folder2 = new File(db.getWorkTree(), "folder2"); + FileUtils.mkdir(folder1); + FileUtils.mkdir(folder2); + File file = new File(folder1, "file1.txt"); + write(file, "folder1--file1.txt"); + file = new File(folder1, "file2.txt"); + write(file, "folder1--file2.txt"); + file = new File(folder2, "file1.txt"); + write(file, "folder--file1.txt"); + file = new File(folder2, "file2.txt"); + write(file, "folder2--file2.txt"); + + Git git = new Git(db); + git.add().addFilepattern(folder1.getName()) + .addFilepattern(folder2.getName()).call(); + RevCommit base = git.commit().setMessage("adding folders").call(); + + recursiveDelete(folder1); + recursiveDelete(folder2); + git.rm().addFilepattern("folder1/file1.txt") + .addFilepattern("folder1/file2.txt") + .addFilepattern("folder2/file1.txt") + .addFilepattern("folder2/file2.txt").call(); + RevCommit other = git.commit() + .setMessage("removing folders on 'branch'").call(); + + git.checkout().setName(base.name()).call(); + + file = new File(folder2, "file3.txt"); + write(file, "folder2--file3.txt"); + + git.add().addFilepattern(folder2.getName()).call(); + git.commit().setMessage("adding another file").call(); + + MergeResult result = git.merge().include(other.getId()) + .setStrategy(MergeStrategy.RESOLVE).call(); + + assertEquals(MergeResult.MergeStatus.MERGED, + result.getMergeStatus()); + assertFalse(folder1.exists()); + } + @Test public void testFileModeMerge() throws Exception { if (!FS.DETECTED.supportsExecute()) 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 6130cc72c..2410d6fe0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -126,6 +126,8 @@ public enum MergeFailureReason { private Map toBeCheckedOut = new HashMap(); + private List toBeDeleted = new ArrayList(); + private Map> mergeResults = new HashMap>(); private Map failingPaths = new HashMap(); @@ -240,16 +242,21 @@ private void checkout() throws NoWorkTreeException, IOException { for (Map.Entry entry : toBeCheckedOut .entrySet()) { File f = new File(db.getWorkTree(), entry.getKey()); - if (entry.getValue() != null) { - createDir(f.getParentFile()); - DirCacheCheckout.checkoutEntry(db, f, entry.getValue(), r); - } else { - if (!f.delete()) - failingPaths.put(entry.getKey(), - MergeFailureReason.COULD_NOT_DELETE); - } + createDir(f.getParentFile()); + DirCacheCheckout.checkoutEntry(db, f, entry.getValue(), r); modifiedFiles.add(entry.getKey()); } + // Iterate in reverse so that "folder/file" is deleted before + // "folder". Otherwise this could result in a failing path because + // of a non-empty directory, for which delete() would fail. + for (int i = toBeDeleted.size() - 1; i >= 0; i--) { + String fileName = toBeDeleted.get(i); + File f = new File(db.getWorkTree(), fileName); + if (!f.delete()) + failingPaths.put(fileName, + MergeFailureReason.COULD_NOT_DELETE); + modifiedFiles.add(fileName); + } } finally { r.release(); } @@ -441,7 +448,7 @@ private boolean processEntry(CanonicalTreeParser base, } else if (modeT == 0 && modeB != 0) { // we want THEIRS ... but THEIRS contains the deletion of the // file - toBeCheckedOut.put(tw.getPathString(), null); + toBeDeleted.add(tw.getPathString()); return true; } }