Let RefDirectory use FileSnapShot to handle fast updates

Since this change may affect performance and memory consumption on every
access to a loose ref I explicitly made it a RFC to collect opinions.

Previously RefDirectory.scanRef() was not detecting an update of a
loose ref when the update didn't changed the modification time of
the backing file. RefDirectory cached loose refs and the way to detect
outdated cache entries was to compare lastmodification timestamp on the
file representing the ref. If two updates to the same ref happen faster
than the filesystem-timer granularity (for linux this is 2 seconds)
there is the possiblity that we don't detect the update.

Because of this bug EGit's PushOperationTest only works with 2 second
sleeps inside.

This change let RefDirectory use FileSnapshot to detect such situations.
FileSnapshot helps to remember when a file was last read from disk and
therefore enables to decide when to load a file from disk although
modification time has not changed.

Change-Id: I03b9a137af097ec69c4c5e2eaa512d2bdd7fe080
Signed-off-by: Christian Halstrick <christian.halstrick@sap.com>
Signed-off-by: Chris Aniszczyk <caniszczyk@gmail.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java

@@ -152,4 +152,47 @@ public class PushCommandTest extends RepositoryTestCase {
 		assertEquals(commit2.getId(), db2.resolve(branch));
 	}
 
+	/**
+	 * Check that pushes over file protocol lead to appropriate ref-updates.
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testPushRefUpdate() throws Exception {
+		Git git = new Git(db);
+		Git git2 = new Git(createBareRepository());
+
+		final StoredConfig config = git.getRepository().getConfig();
+		RemoteConfig remoteConfig = new RemoteConfig(config, "test");
+		URIish uri = new URIish(git2.getRepository().getDirectory().toURI()
+				.toURL());
+		remoteConfig.addURI(uri);
+		remoteConfig.addPushRefSpec(new RefSpec("+refs/heads/*:refs/heads/*"));
+		remoteConfig.update(config);
+		config.save();
+
+		writeTrashFile("f", "content of f");
+		git.add().addFilepattern("f").call();
+		RevCommit commit = git.commit().setMessage("adding f").call();
+
+		assertEquals(null, git2.getRepository().resolve("refs/heads/master"));
+		git.push().setRemote("test").call();
+		assertEquals(commit.getId(),
+				git2.getRepository().resolve("refs/heads/master"));
+
+		git.branchCreate().setName("refs/heads/test").call();
+		git.checkout().setName("refs/heads/test").call();
+
+
+		for (int i = 0; i < 6; i++) {
+			writeTrashFile("f" + i, "content of f" + i);
+			git.add().addFilepattern("f" + i).call();
+			commit = git.commit().setMessage("adding f" + i).call();
+			git.push().setRemote("test").call();
+			git2.getRepository().getAllRefs();
+			assertEquals("failed to update on attempt " + i, commit.getId(),
+					git2.getRepository().resolve("refs/heads/test"));
+
+		}
+	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java

@@ -169,7 +169,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		Ref head;
 
 		writeLooseRef(HEAD, A);
-		BUG_WorkAroundRacyGitIssues(HEAD);
 
 		all = refdir.getRefs(RefDatabase.ALL);
 		assertEquals(1, all.size());
@@ -190,7 +189,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 
 		writeLooseRef(HEAD, A);
 		writeLooseRef("refs/heads/master", B);
-		BUG_WorkAroundRacyGitIssues(HEAD);
 
 		all = refdir.getRefs(RefDatabase.ALL);
 		assertEquals(2, all.size());
@@ -328,7 +326,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		assertTrue(heads.containsKey("refs/heads/C"));
 
 		writeLooseRef("refs/heads/B", "FAIL\n");
-		BUG_WorkAroundRacyGitIssues("refs/heads/B");
 
 		heads = refdir.getRefs(RefDatabase.ALL);
 		assertEquals(2, heads.size());
@@ -547,7 +544,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		assertEquals(A, all.get(HEAD).getObjectId());
 
 		writeLooseRef("refs/heads/master", B);
-		BUG_WorkAroundRacyGitIssues("refs/heads/master");
 		all = refdir.getRefs(RefDatabase.ALL);
 		assertEquals(B, all.get(HEAD).getObjectId());
 	}
@@ -561,7 +557,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		assertEquals(A, all.get(HEAD).getObjectId());
 
 		writeLooseRef("refs/heads/master", B);
-		BUG_WorkAroundRacyGitIssues("refs/heads/master");
 
 		Ref master = refdir.getRef("refs/heads/master");
 		assertEquals(B, master.getObjectId());
@@ -760,7 +755,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 
 		writeLooseRef("refs/5", "ref: refs/6\n");
 		writeLooseRef("refs/6", "ref: refs/end\n");
-		BUG_WorkAroundRacyGitIssues("refs/5");
 		all = refdir.getRefs(RefDatabase.ALL);
 		r = all.get("refs/1");
 		assertNull("mising 1 due to cycle", r);
@@ -1078,23 +1072,4 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
 		File path = new File(diskRepo.getDirectory(), name);
 		assertTrue("deleted " + name, path.delete());
 	}
-
-	/**
-	 * Kick the timestamp of a local file.
-	 * <p>
-	 * We shouldn't have to make these method calls. The cache is using file
-	 * system timestamps, and on many systems unit tests run faster than the
-	 * modification clock. Dumping the cache after we make an edit behind
-	 * RefDirectory's back allows the tests to pass.
-	 *
-	 * @param name
-	 *            the file in the repository to force a time change on.
-	 */
-	private void BUG_WorkAroundRacyGitIssues(String name) {
-		File path = new File(diskRepo.getDirectory(), name);
-		long old = path.lastModified();
-		long set = 1250379778668L; // Sat Aug 15 20:12:58 GMT-03:30 2009
-		path.setLastModified(set);
-		assertTrue("time changed", old != path.lastModified());
-	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java

@@ -101,6 +101,22 @@ public class FileSnapshot {
 		return new FileSnapshot(read, modified);
 	}
 
+	/**
+	 * Record a snapshot for a file for which the last modification time is
+	 * already known.
+	 * <p>
+	 * This method should be invoked before the file is accessed.
+	 *
+	 * @param modified
+	 *            the last modification time of the file
+	 *
+	 * @return the snapshot.
+	 */
+	public static FileSnapshot save(long modified) {
+		final long read = System.currentTimeMillis();
+		return new FileSnapshot(read, modified);
+	}
+
 	/** Last observed modification time of the path. */
 	private final long lastModified;
 

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java

@@ -842,19 +842,22 @@ public class RefDirectory extends RefDatabase {
 		return n;
 	}
 
+	@SuppressWarnings("null")
 	private LooseRef scanRef(LooseRef ref, String name) throws IOException {
 		final File path = fileFor(name);
-		final long modified = path.lastModified();
+		FileSnapshot currentSnapshot = null;
 
 		if (ref != null) {
-			if (ref.getLastModified() == modified)
+			currentSnapshot = ref.getSnapShot();
+			if (!currentSnapshot.isModified(path))
 				return ref;
 			name = ref.getName();
-		} else if (modified == 0)
+		} else if (!path.exists())
 			return null;
 
 		final int limit = 4096;
 		final byte[] buf;
+		FileSnapshot otherSnapshot = FileSnapshot.save(path);
 		try {
 			buf = IO.readSome(path, limit);
 		} catch (FileNotFoundException noFile) {
@@ -877,7 +880,12 @@ public class RefDirectory extends RefDatabase {
 				throw new IOException(MessageFormat.format(JGitText.get().notARef, name, content));
 			}
 			final String target = RawParseUtils.decode(buf, 5, n);
-			return newSymbolicRef(modified, name, target);
+			if (ref != null && ref.isSymbolic()
+					&& ref.getTarget().getName().equals(target)) {
+				currentSnapshot.setClean(otherSnapshot);
+				return ref;
+			}
+			return newSymbolicRef(path.lastModified(), name, target);
 		}
 
 		if (n < OBJECT_ID_STRING_LENGTH)
@@ -886,13 +894,19 @@ public class RefDirectory extends RefDatabase {
 		final ObjectId id;
 		try {
 			id = ObjectId.fromString(buf, 0);
+			if (ref != null && !ref.isSymbolic()
+					&& ref.getTarget().getObjectId().equals(id)) {
+				currentSnapshot.setClean(otherSnapshot);
+				return ref;
+			}
+
 		} catch (IllegalArgumentException notRef) {
 			while (0 < n && Character.isWhitespace(buf[n - 1]))
 				n--;
 			String content = RawParseUtils.decode(buf, 0, n);
 			throw new IOException(MessageFormat.format(JGitText.get().notARef, name, content));
 		}
-		return new LooseUnpeeled(modified, name, id);
+		return new LooseUnpeeled(path.lastModified(), name, id);
 	}
 
 	private static boolean isSymRef(final byte[] buf, int n) {
@@ -997,22 +1011,22 @@ public class RefDirectory extends RefDatabase {
 	}
 
 	private static interface LooseRef extends Ref {
-		long getLastModified();
+		FileSnapshot getSnapShot();
 
 		LooseRef peel(ObjectIdRef newLeaf);
 	}
 
 	private final static class LoosePeeledTag extends ObjectIdRef.PeeledTag
 			implements LooseRef {
-		private final long lastModified;
+		private final FileSnapshot snapShot;
 
 		LoosePeeledTag(long mtime, String refName, ObjectId id, ObjectId p) {
 			super(LOOSE, refName, id, p);
-			this.lastModified = mtime;
+			snapShot = FileSnapshot.save(mtime);
 		}
 
-		public long getLastModified() {
+		public FileSnapshot getSnapShot() {
-			return lastModified;
+			return snapShot;
 		}
 
 		public LooseRef peel(ObjectIdRef newLeaf) {
@@ -1022,15 +1036,15 @@ public class RefDirectory extends RefDatabase {
 
 	private final static class LooseNonTag extends ObjectIdRef.PeeledNonTag
 			implements LooseRef {
-		private final long lastModified;
+		private final FileSnapshot snapShot;
 
 		LooseNonTag(long mtime, String refName, ObjectId id) {
 			super(LOOSE, refName, id);
-			this.lastModified = mtime;
+			snapShot = FileSnapshot.save(mtime);
 		}
 
-		public long getLastModified() {
+		public FileSnapshot getSnapShot() {
-			return lastModified;
+			return snapShot;
 		}
 
 		public LooseRef peel(ObjectIdRef newLeaf) {
@@ -1040,37 +1054,38 @@ public class RefDirectory extends RefDatabase {
 
 	private final static class LooseUnpeeled extends ObjectIdRef.Unpeeled
 			implements LooseRef {
-		private final long lastModified;
+		private final FileSnapshot snapShot;
 
 		LooseUnpeeled(long mtime, String refName, ObjectId id) {
 			super(LOOSE, refName, id);
-			this.lastModified = mtime;
+			snapShot = FileSnapshot.save(mtime);
 		}
 
-		public long getLastModified() {
+		public FileSnapshot getSnapShot() {
-			return lastModified;
+			return snapShot;
 		}
 
 		public LooseRef peel(ObjectIdRef newLeaf) {
 			if (newLeaf.getPeeledObjectId() != null)
-				return new LoosePeeledTag(lastModified, getName(),
+				return new LoosePeeledTag(snapShot.lastModified(), getName(),
 						getObjectId(), newLeaf.getPeeledObjectId());
 			else
-				return new LooseNonTag(lastModified, getName(), getObjectId());
+				return new LooseNonTag(snapShot.lastModified(), getName(),
+						getObjectId());
 		}
 	}
 
 	private final static class LooseSymbolicRef extends SymbolicRef implements
 			LooseRef {
-		private final long lastModified;
+		private final FileSnapshot snapShot;
 
 		LooseSymbolicRef(long mtime, String refName, Ref target) {
 			super(refName, target);
-			this.lastModified = mtime;
+			snapShot = FileSnapshot.save(mtime);
 		}
 
-		public long getLastModified() {
+		public FileSnapshot getSnapShot() {
-			return lastModified;
+			return snapShot;
 		}
 
 		public LooseRef peel(ObjectIdRef newLeaf) {