From 2302a6d3ceea2926e785dba57abd58f609684e86 Mon Sep 17 00:00:00 2001 From: Christian Halstrick Date: Fri, 20 May 2011 11:18:20 +0200 Subject: [PATCH] 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 Signed-off-by: Chris Aniszczyk --- .../org/eclipse/jgit/api/PushCommandTest.java | 43 +++++++++++++ .../jgit/storage/file/RefDirectoryTest.java | 25 -------- .../jgit/storage/file/FileSnapshot.java | 16 +++++ .../jgit/storage/file/RefDirectory.java | 63 ++++++++++++------- 4 files changed, 98 insertions(+), 49 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java index 6a28605ed..760d31a34 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java +++ b/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")); + + } + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java index 56e5549b8..dc2ccb97f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java +++ b/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. - *

- * 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()); - } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java index bbec80c86..ce556a792 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java +++ b/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. + *

+ * 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; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java index cd199dcf9..90fd38bde 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java +++ b/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() { - return lastModified; + public FileSnapshot getSnapShot() { + 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() { - return lastModified; + public FileSnapshot getSnapShot() { + 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() { - return lastModified; + public FileSnapshot getSnapShot() { + 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() { - return lastModified; + public FileSnapshot getSnapShot() { + return snapShot; } public LooseRef peel(ObjectIdRef newLeaf) {