From 955b024995a423c32154b73f99cff606d882bbe4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 24 Jul 2012 20:07:48 -0400 Subject: [PATCH] Revert "Teach ResolveMerger to create more correct DirCacheEntry's" This reverts commit 3ea694c2523d909190b5350e13254a62e94ec5d5 Merges with unmodified subtrees are broken with this commit present. Back it out until a fixed version can be supplied. --- .../eclipse/jgit/lib/RepositoryTestCase.java | 29 --- .../eclipse/jgit/merge/ResolveMergerTest.java | 178 +----------------- .../org/eclipse/jgit/merge/ResolveMerger.java | 100 +++------- 3 files changed, 27 insertions(+), 280 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java index c06322e8e..0c573ebe7 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java @@ -397,33 +397,4 @@ public abstract class RepositoryTestCase extends LocalDiskRepositoryTestCase { RefUpdate refUpdate = db.updateRef(Constants.HEAD); refUpdate.link(branchName); } - - /** - * Writes a number of files in the working tree. The first content specified - * will be written into a file named '0', the second into a file named "1" - * and so on. If null is specified as content then this file is - * skipped. - * - * @param ensureDistinctTimestamps - * if set to true then between two write operations - * this method will wait to ensure that the second file will get - * a different lastmodification timestamp than the first file. - * @param contents - * the contents which should be written into the files - * @return the File object associated to the last written file. - * @throws IOException - * @throws InterruptedException - */ - protected File writeTrashFiles(boolean ensureDistinctTimestamps, - String... contents) - throws IOException, InterruptedException { - File f = null; - for (int i = 0; i < contents.length; i++) - if (contents[i] != null) { - if (ensureDistinctTimestamps && (f != null)) - fsTick(f); - f = writeTrashFile(Integer.toString(i), contents[i]); - } - return f; - } } 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 index 95497d81e..4cb089602 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java @@ -42,18 +42,12 @@ */ package org.eclipse.jgit.merge; -import static org.junit.Assert.*; +import static org.junit.Assert.assertFalse; import java.io.File; -import java.io.FileInputStream; -import java.io.IOException; import org.eclipse.jgit.api.Git; -import org.eclipse.jgit.api.MergeResult; -import org.eclipse.jgit.api.MergeResult.MergeStatus; -import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.lib.RepositoryTestCase; -import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.util.FileUtils; @@ -101,174 +95,4 @@ public class ResolveMergerTest extends RepositoryTestCase { assertFalse(ok); } - @Test - public void checkLockedFilesToBeDeleted() throws Exception { - Git git = Git.wrap(db); - - writeTrashFile("a.txt", "orig"); - writeTrashFile("b.txt", "orig"); - git.add().addFilepattern("a.txt").addFilepattern("b.txt").call(); - RevCommit first = git.commit().setMessage("added a.txt, b.txt").call(); - - // modify and delete files on the master branch - writeTrashFile("a.txt", "master"); - git.rm().addFilepattern("b.txt").call(); - RevCommit masterCommit = git.commit() - .setMessage("modified a.txt, deleted b.txt").setAll(true) - .call(); - - // switch back to a side branch - git.checkout().setCreateBranch(true).setStartPoint(first) - .setName("side").call(); - writeTrashFile("c.txt", "side"); - git.add().addFilepattern("c.txt").call(); - git.commit().setMessage("added c.txt").call(); - - // Get a handle to the the file so on windows it can't be deleted. - FileInputStream fis = new FileInputStream(new File(db.getWorkTree(), - "b.txt")); - MergeResult mergeRes = git.merge().include(masterCommit).call(); - if (mergeRes.getMergeStatus().equals(MergeStatus.FAILED)) { - // probably windows - assertEquals(1, mergeRes.getFailingPaths().size()); - assertEquals(MergeFailureReason.COULD_NOT_DELETE, mergeRes - .getFailingPaths().get("b.txt")); - } - assertEquals("[a.txt, mode:100644, content:master]" - + "[c.txt, mode:100644, content:side]", indexState(CONTENT)); - fis.close(); - } - - @Test - public void checkForCorrectIndex() throws Exception { - File f; - long lastTs4, lastTsIndex; - Git git = Git.wrap(db); - File indexFile = db.getIndexFile(); - - // Create initial content and remember when the last file was written. - f = writeTrashFiles(false, "orig", "orig", "1\n2\n3", "orig", "orig"); - lastTs4 = f.lastModified(); - - // add all files, commit and check this doesn't update any working tree - // files and that the index is in a new file system timer tick. Make - // sure to wait long enough before adding so the index doesn't contain - // racily clean entries - fsTick(f); - git.add().addFilepattern(".").call(); - RevCommit firstCommit = git.commit().setMessage("initial commit") - .call(); - checkConsistentLastModified("0", "1", "2", "3", "4"); - checkModificationTimeStampOrder("1", "2", "3", "4", "<.git/index"); - assertEquals("Commit should not touch working tree file 4", lastTs4, - new File(db.getWorkTree(), "4").lastModified()); - lastTsIndex = indexFile.lastModified(); - - // Do modifications on the master branch. Then add and commit. This - // should touch only "0", "2 and "3" - fsTick(indexFile); - f = writeTrashFiles(false, "master", null, "1master\n2\n3", "master", - null); - fsTick(f); - git.add().addFilepattern(".").call(); - RevCommit masterCommit = git.commit().setMessage("master commit") - .call(); - checkConsistentLastModified("0", "1", "2", "3", "4"); - checkModificationTimeStampOrder("1", "4", "*" + lastTs4, "<*" - + lastTsIndex, "<0", - "2", "3", "<.git/index"); - lastTsIndex = indexFile.lastModified(); - - // Checkout a side branch. This should touch only "0", "2 and "3" - fsTick(indexFile); - git.checkout().setCreateBranch(true).setStartPoint(firstCommit) - .setName("side").call(); - checkConsistentLastModified("0", "1", "2", "3", "4"); - checkModificationTimeStampOrder("1", "4", "*" + lastTs4, "<*" - + lastTsIndex, "<0", - "2", "3", ".git/index"); - lastTsIndex = indexFile.lastModified(); - - // This checkout may have populated worktree and index so fast that we - // may have smudged entries now. Check that we have the right content - // and then rewrite the index to get rid of smudged state - assertEquals("[0, mode:100644, content:orig]" // - + "[1, mode:100644, content:orig]" // - + "[2, mode:100644, content:1\n2\n3]" // - + "[3, mode:100644, content:orig]" // - + "[4, mode:100644, content:orig]", // - indexState(CONTENT)); - fsTick(indexFile); - f = writeTrashFiles(false, "orig", "orig", "1\n2\n3", "orig", "orig"); - lastTs4 = f.lastModified(); - fsTick(f); - git.add().addFilepattern(".").call(); - checkConsistentLastModified("0", "1", "2", "3", "4"); - checkModificationTimeStampOrder("*" + lastTsIndex, "<0", "1", "2", "3", - "4", "<.git/index"); - lastTsIndex = indexFile.lastModified(); - - // Do modifications on the side branch. Touch only "1", "2 and "3" - fsTick(indexFile); - f = writeTrashFiles(false, null, "side", "1\n2\n3side", "side", null); - fsTick(f); - git.add().addFilepattern(".").call(); - git.commit().setMessage("side commit").call(); - checkConsistentLastModified("0", "1", "2", "3", "4"); - checkModificationTimeStampOrder("0", "4", "*" + lastTs4, "<*" - + lastTsIndex, "<1", "2", "3", "<.git/index"); - lastTsIndex = indexFile.lastModified(); - - // merge master and side. Should only touch "0," "2" and "3" - fsTick(indexFile); - git.merge().include(masterCommit).call(); - checkConsistentLastModified("0", "1", "2", "4"); - checkModificationTimeStampOrder("4", "*" + lastTs4, "<1", "<*" - + lastTsIndex, "<0", "2", "3", ".git/index"); - assertEquals( - "[0, mode:100644, content:master]" // - + "[1, mode:100644, content:side]" // - + "[2, mode:100644, content:1master\n2\n3side\n]" // - + "[3, mode:100644, stage:1, content:orig][3, mode:100644, stage:2, content:side][3, mode:100644, stage:3, content:master]" // - + "[4, mode:100644, content:orig]", // - indexState(CONTENT)); - } - - // Assert that every specified index entry has the same last modification - // timestamp as the associated file - private void checkConsistentLastModified(String... pathes) - throws IOException { - DirCache dc = db.readDirCache(); - File workTree = db.getWorkTree(); - for (String path : pathes) - assertEquals( - "IndexEntry with path " - + path - + " has lastmodified with is different from the worktree file", - new File(workTree, path).lastModified(), dc.getEntry(path) - .getLastModified()); - } - - // Assert that modification timestamps of working tree files are as - // expected. You may specify n files. It is asserted that every file - // i+1 is not older than file i. If a path of file i+1 is prefixed with "<" - // then this file must be younger then file i. A path "*" - // represents a file with a modification time of - // E.g. ("a", "b", " lastMod); - else - assertTrue("path " + p + " is older than predecesssor", - curMod >= lastMod); - } - } } 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 3034bfb5d..2410d6fe0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -204,13 +204,6 @@ public class ResolveMerger extends ThreeWayMerger { } if (!inCore) { - // No problem found. The only thing left to be done is to - // checkout - // all files from "theirs" which have been selected to go into - // the - // new index. - checkout(); - // All content-merges are successfully done. If we can now write the // new index we are on quite safe ground. Even if the checkout of // files coming from "theirs" fails the user can work around such @@ -221,6 +214,10 @@ public class ResolveMerger extends ThreeWayMerger { } builder = null; + // No problem found. The only thing left to be done is to checkout + // all files from "theirs" which have been selected to go into the + // new index. + checkout(); } else { builder.finish(); builder = null; @@ -316,43 +313,19 @@ public class ResolveMerger extends ThreeWayMerger { * @param path * @param p * @param stage - * @param lastMod - * @param len * @return the entry which was added to the index */ - private DirCacheEntry add(byte[] path, CanonicalTreeParser p, int stage, - long lastMod, long len) { + private DirCacheEntry add(byte[] path, CanonicalTreeParser p, int stage) { if (p != null && !p.getEntryFileMode().equals(FileMode.TREE)) { DirCacheEntry e = new DirCacheEntry(path, stage); e.setFileMode(p.getEntryFileMode()); e.setObjectId(p.getEntryObjectId()); - e.setLastModified(lastMod); - e.setLength(len); builder.add(e); return e; } return null; } - /** - * adds a entry to the index builder which is a copy of the specified - * DirCacheEntry - * - * @param e - * the entry which should be copied - * - * @return the entry which was added to the index - */ - private DirCacheEntry keep(DirCacheEntry e) { - DirCacheEntry newEntry = new DirCacheEntry(e.getPathString(), e.getStage()); - newEntry.setFileMode(e.getFileMode()); - newEntry.setObjectId(e.getObjectId()); - newEntry.setLastModified(e.getLastModified()); - newEntry.setLength(e.getLength()); - builder.add(newEntry); - return newEntry; - } - /** * Processes one path and tries to merge. This method will do all do all * trivial (not content) merges and will also detect if a merge will fail. @@ -409,27 +382,12 @@ public class ResolveMerger extends ThreeWayMerger { if (isIndexDirty()) return false; - DirCacheEntry ourDce = null; - - if (index == null) { - // create a fake DCE, but only if ours is valid. ours is kept only - // in case it is valid, so a null ourDce is ok in all other cases. - if (modeO != 0) { - ourDce = new DirCacheEntry(tw.getRawPath()); - ourDce.setObjectId(tw.getObjectId(T_OURS)); - ourDce.setFileMode(tw.getFileMode(T_OURS)); - } - } else { - ourDce = index.getDirCacheEntry(); - } - if (nonTree(modeO) && nonTree(modeT) && tw.idEqual(T_OURS, T_THEIRS)) { // OURS and THEIRS have equal content. Check the file mode if (modeO == modeT) { // content and mode of OURS and THEIRS are equal: it doesn't - // matter which one we choose. OURS is chosen. Since the index - // is clean (the index matches already OURS) we can keep the existing one - keep(ourDce); + // matter which one we choose. OURS is chosen. + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0); // no checkout needed! return true; } else { @@ -440,25 +398,22 @@ public class ResolveMerger extends ThreeWayMerger { if (newMode != FileMode.MISSING.getBits()) { if (newMode == modeO) // ours version is preferred - keep(ourDce); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0); else { // the preferred version THEIRS has a different mode // than ours. Check it out! if (isWorktreeDirty(work)) return false; - // we know about length and lastMod only after we have written the new content. - // This will happen later. Set these values to 0 for know. DirCacheEntry e = add(tw.getRawPath(), theirs, - DirCacheEntry.STAGE_0, 0, 0); + DirCacheEntry.STAGE_0); toBeCheckedOut.put(tw.getPathString(), e); } return true; } else { - // FileModes are not mergeable. We found a conflict on modes. - // For conflicting entries we don't know lastModified and length. - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); - add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0); - add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0); + // FileModes are not mergeable. We found a conflict on modes + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3); unmergedPaths.add(tw.getPathString()); mergeResults.put( tw.getPathString(), @@ -471,8 +426,8 @@ public class ResolveMerger extends ThreeWayMerger { if (nonTree(modeO) && modeB == modeT && tw.idEqual(T_BASE, T_THEIRS)) { // THEIRS was not changed compared to BASE. All changes must be in - // OURS. OURS is chosen. We can keep the existing entry. - keep(ourDce); + // OURS. OURS is chosen. + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0); // no checkout needed! return true; } @@ -485,11 +440,8 @@ public class ResolveMerger extends ThreeWayMerger { if (isWorktreeDirty(work)) return false; if (nonTree(modeT)) { - // we know about length and lastMod only after we have written - // the new content. - // This will happen later. Set these values to 0 for know. DirCacheEntry e = add(tw.getRawPath(), theirs, - DirCacheEntry.STAGE_0, 0, 0); + DirCacheEntry.STAGE_0); if (e != null) toBeCheckedOut.put(tw.getPathString(), e); return true; @@ -508,16 +460,16 @@ public class ResolveMerger extends ThreeWayMerger { // detected later if (nonTree(modeO) && !nonTree(modeT)) { if (nonTree(modeB)) - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); - add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0); + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2); unmergedPaths.add(tw.getPathString()); enterSubtree = false; return true; } if (nonTree(modeT) && !nonTree(modeO)) { if (nonTree(modeB)) - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); - add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0); + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3); unmergedPaths.add(tw.getPathString()); enterSubtree = false; return true; @@ -550,10 +502,10 @@ public class ResolveMerger extends ThreeWayMerger { if (((modeO != 0 && !tw.idEqual(T_BASE, T_OURS)) || (modeT != 0 && !tw .idEqual(T_BASE, T_THEIRS)))) { - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); - add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0); + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2); DirCacheEntry e = add(tw.getRawPath(), theirs, - DirCacheEntry.STAGE_3, 0, 0); + DirCacheEntry.STAGE_3); // OURS was deleted checkout THEIRS if (modeO == 0) { @@ -657,9 +609,9 @@ public class ResolveMerger extends ThreeWayMerger { // a conflict occurred, the file will contain conflict markers // the index will be populated with the three stages and only the // workdir (if used) contains the halfways merged content - add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0); - add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0); - add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0); + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3); mergeResults.put(tw.getPathString(), result); } else { // no conflict occurred, the file will contain fully merged content.