Teach ResolveMerger to create more correct DirCacheEntry's

Currently, after a merge/cherry-pick/rebase, all index entries are
smudged as the ResolveMerger never sets entry lengths and/or
modification times. This change teaches it to re-set them at least for
things it did not touch. The other entries are then repaired when the
index is persisted, or entries are checked out.

Change-Id: I0944f2017483d32043d0d09409b13055b5609a4b
Signed-off-by: Christian Halstrick <christian.halstrick@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java

@@ -397,4 +397,33 @@ 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 <code>null</code> is specified as content then this file is
+	 * skipped.
+	 *
+	 * @param ensureDistinctTimestamps
+	 *            if set to <code>true</code> 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;
+			}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/ResolveMergerTest.java

@@ -42,12 +42,18 @@
  */
 package org.eclipse.jgit.merge;
 
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.*;
 
 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;
@@ -95,4 +101,174 @@ 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 "*<modtime>"
+	// represents a file with a modification time of <modtime>
+	// E.g. ("a", "b", "<c", "f/a.txt") means: a<=b<c<=f/a.txt
+	private void checkModificationTimeStampOrder(String... pathes) {
+		long lastMod = Long.MIN_VALUE;
+		for (String p : pathes) {
+			boolean strong = p.startsWith("<");
+			boolean fixed = p.charAt(strong ? 1 : 0) == '*';
+			p = p.substring((strong ? 1 : 0) + (fixed ? 1 : 0));
+			long curMod = fixed ? Long.valueOf(p).longValue() : new File(
+					db.getWorkTree(), p).lastModified();
+			if (strong)
+				assertTrue("path " + p + " is not younger than predecesssor",
+						curMod > lastMod);
+			else
+				assertTrue("path " + p + " is older than predecesssor",
+						curMod >= lastMod);
+		}
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java

@@ -204,6 +204,13 @@ 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
@@ -214,10 +221,6 @@ 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;
@@ -313,19 +316,43 @@ 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) {
+	private DirCacheEntry add(byte[] path, CanonicalTreeParser p, int stage,
+			long lastMod, long len) {
 		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.
@@ -382,12 +409,27 @@ 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.
+				// matter which one we choose. OURS is chosen. Since the index
-				add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0);
+				// is clean (the index matches already OURS) we can keep the existing one
+				keep(ourDce);
 				// no checkout needed!
 				return true;
 			} else {
@@ -398,22 +440,25 @@ public class ResolveMerger extends ThreeWayMerger {
 				if (newMode != FileMode.MISSING.getBits()) {
 					if (newMode == modeO)
 						// ours version is preferred
-						add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0);
+						keep(ourDce);
 					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);
+								DirCacheEntry.STAGE_0, 0, 0);
 						toBeCheckedOut.put(tw.getPathString(), e);
 					}
 					return true;
 				} else {
-					// FileModes are not mergeable. We found a conflict on modes
+					// FileModes are not mergeable. We found a conflict on modes.
-					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1);
+					// For conflicting entries we don't know lastModified and length.
-					add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2);
+					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0);
-					add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3);
+					add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0);
+					add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0);
 					unmergedPaths.add(tw.getPathString());
 					mergeResults.put(
 							tw.getPathString(),
@@ -426,8 +471,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.
+			// OURS. OURS is chosen. We can keep the existing entry.
-			add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0);
+			keep(ourDce);
 			// no checkout needed!
 			return true;
 		}
@@ -440,8 +485,11 @@ 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);
+						DirCacheEntry.STAGE_0, 0, 0);
 				if (e != null)
 					toBeCheckedOut.put(tw.getPathString(), e);
 				return true;
@@ -460,16 +508,16 @@ public class ResolveMerger extends ThreeWayMerger {
 			// detected later
 			if (nonTree(modeO) && !nonTree(modeT)) {
 				if (nonTree(modeB))
-					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1);
+					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0);
-				add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2);
+				add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0);
 				unmergedPaths.add(tw.getPathString());
 				enterSubtree = false;
 				return true;
 			}
 			if (nonTree(modeT) && !nonTree(modeO)) {
 				if (nonTree(modeB))
-					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1);
+					add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0);
-				add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3);
+				add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0);
 				unmergedPaths.add(tw.getPathString());
 				enterSubtree = false;
 				return true;
@@ -502,10 +550,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);
+				add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0);
-				add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2);
+				add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0);
 				DirCacheEntry e = add(tw.getRawPath(), theirs,
-						DirCacheEntry.STAGE_3);
+						DirCacheEntry.STAGE_3, 0, 0);
 
 				// OURS was deleted checkout THEIRS
 				if (modeO == 0) {
@@ -609,9 +657,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);
+			add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, 0, 0);
-			add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2);
+			add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, 0, 0);
-			add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3);
+			add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, 0, 0);
 			mergeResults.put(tw.getPathString(), result);
 		} else {
 			// no conflict occurred, the file will contain fully merged content.