From 43b06f51f96dddc3d512904f9e1f0bb6b5cc5984 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 10 May 2019 00:58:42 +0200 Subject: [PATCH] Capture reason for result of FileSnapshot#isModified This allows to verify the expected behavior in FileSnapshotTest#testSimulatePackfileReplacement and enables extending FileSnapshot for packfiles to read the packfile's checksum as another criterion to detect modifications without reading the full content. Also add another field capturing the result of the last check if lastModified was racily clean. Remove unnecessary determination of raciness in the constructor. It was determined twice in all relevant cases. Change-Id: I100a2f49d7949693d7b72daa89437e166f1dc107 Signed-off-by: Matthias Sohn --- .../storage/file/FileSnapshotTest.java | 7 ++ .../internal/storage/file/FileSnapshot.java | 79 +++++++++++++++---- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileSnapshotTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileSnapshotTest.java index 6ef87d8b0..79ede9691 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileSnapshotTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileSnapshotTest.java @@ -42,6 +42,7 @@ */ package org.eclipse.jgit.internal.storage.file; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; @@ -154,6 +155,12 @@ public void testSimulatePackfileReplacement() throws Exception { StandardCopyOption.ATOMIC_MOVE); Files.setLastModifiedTime(f1.toPath(), timestamp); assertTrue(save.isModified(f1)); + assertTrue("unexpected change of fileKey", save.wasFileKeyChanged()); + assertFalse("unexpected size change", save.wasSizeChanged()); + assertFalse("unexpected lastModified change", + save.wasLastModifiedChanged()); + assertFalse("lastModified was unexpectedly racily clean", + save.wasLastModifiedRacilyClean()); } private File createFile(String string) throws IOException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java index 09e15938e..23cd5ba31 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java @@ -187,13 +187,20 @@ public static FileSnapshot save(long modified) { */ private Object fileKey; + private boolean sizeChanged; + + private boolean fileKeyChanged; + + private boolean lastModifiedChanged; + + private boolean wasRacyClean; + private FileSnapshot(long read, long modified, long size, @NonNull Duration fsTimestampResolution, @NonNull Object fileKey) { this.lastRead = read; this.lastModified = modified; this.fsTimestampResolution = fsTimestampResolution; this.size = size; - this.cannotBeRacilyClean = notRacyClean(read); this.fileKey = fileKey; } @@ -234,9 +241,19 @@ public boolean isModified(File path) { currSize = path.length(); currFileKey = MISSING_FILEKEY; } - return isSizeChanged(currSize) - || isFileKeyChanged(currFileKey) - || isModified(currLastModified); + sizeChanged = isSizeChanged(currSize); + if (sizeChanged) { + return true; + } + fileKeyChanged = isFileKeyChanged(currFileKey); + if (fileKeyChanged) { + return true; + } + lastModifiedChanged = isModified(currLastModified); + if (lastModifiedChanged) { + return true; + } + return false; } /** @@ -263,8 +280,9 @@ public boolean isModified(File path) { */ public void setClean(FileSnapshot other) { final long now = other.lastRead; - if (notRacyClean(now)) + if (!isRacyClean(now)) { cannotBeRacilyClean = true; + } lastRead = now; } @@ -303,6 +321,38 @@ public int hashCode() { fileKey); } + /** + * @return {@code true} if FileSnapshot.isModified(File) found the file size + * changed + */ + boolean wasSizeChanged() { + return sizeChanged; + } + + /** + * @return {@code true} if FileSnapshot.isModified(File) found the file key + * changed + */ + boolean wasFileKeyChanged() { + return fileKeyChanged; + } + + /** + * @return {@code true} if FileSnapshot.isModified(File) found the file's + * lastModified changed + */ + boolean wasLastModifiedChanged() { + return lastModifiedChanged; + } + + /** + * @return {@code true} if FileSnapshot.isModified(File) detected that + * lastModified is racily clean + */ + boolean wasLastModifiedRacilyClean() { + return wasRacyClean; + } + /** {@inheritDoc} */ @SuppressWarnings("nls") @Override @@ -320,37 +370,36 @@ public String toString() { + ", fileKey: " + fileKey + "]"; } - private boolean notRacyClean(long read) { + private boolean isRacyClean(long read) { // add a 10% safety margin long racyNanos = (fsTimestampResolution.toNanos() + 1) * 11 / 10; - return (read - lastModified) * 1_000_000 > racyNanos; + return wasRacyClean = (read - lastModified) * 1_000_000 <= racyNanos; } private boolean isModified(long currLastModified) { // Any difference indicates the path was modified. - // - if (lastModified != currLastModified) + + lastModifiedChanged = lastModified != currLastModified; + if (lastModifiedChanged) { return true; + } // We have already determined the last read was far enough // after the last modification that any new modifications // are certain to change the last modified time. - // - if (cannotBeRacilyClean) + if (cannotBeRacilyClean) { return false; - - if (notRacyClean(lastRead)) { + } + if (!isRacyClean(lastRead)) { // Our last read should have marked cannotBeRacilyClean, // but this thread may not have seen the change. The read // of the volatile field lastRead should have fixed that. - // return false; } // We last read this path too close to its last observed // modification time. We may have missed a modification. // Scan again, to ensure we still see the same state. - // return true; }