From 91101414ae1378cd6a0a6d2673e0e66f4a858828 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 7 May 2019 23:55:54 +0200 Subject: [PATCH] Include filekey file attribute when comparing FileSnapshots Due to finite filesystem timestamp resolution the last modified timestamp of files cannot detect file changes which happened in the immediate past (less than one filesystem timer tick ago). Some filesystems expose unique file identifiers, e.g. inodes in Posix filesystems which are named filekeys in Java's BasicFileAttributes. Use them as another means to detect file modifications based on stat information. Running git gc on a repository yields a new packfile with the same id as a packfile which existed before the gc if these packfiles contain the same set of objects. The content of the old and the new packfile might differ if a different PackConfig was used when writing the packfile. Considering filekeys in FileSnapshot may help to detect such packfile modifications. Bug: 546891 Change-Id: I711a80328c55e1a31171d540880b8e80ec1fe095 Signed-off-by: Matthias Sohn --- .../storage/file/FileSnapshotTest.java | 28 ++++++++++ .../internal/storage/file/FileSnapshot.java | 52 +++++++++++++++---- 2 files changed, 71 insertions(+), 9 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 9ceaa345d..91273362f 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 @@ -47,6 +47,9 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.List; @@ -127,6 +130,31 @@ public void testNewFileNoWait() throws Exception { assertTrue(save.isModified(f1)); } + /** + * Simulate packfile replacement in same file which may occur if set of + * objects in the pack is the same but pack config was different. On Posix + * filesystems this should change the inode (filekey in java.nio + * terminology). + * + * @throws Exception + */ + @Test + public void testSimulatePackfileReplacement() throws Exception { + File f1 = createFile("file"); // inode y + File f2 = createFile("fool"); // Guarantees new inode x + // wait on f2 since this method resets lastModified of the file + // and leaves lastModified of f1 untouched + waitNextSec(f2); + waitNextSec(f2); + FileTime timestamp = Files.getLastModifiedTime(f1.toPath()); + FileSnapshot save = FileSnapshot.save(f1); + Files.move(f2.toPath(), f1.toPath(), // Now "file" is inode x + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + Files.setLastModifiedTime(f1.toPath(), timestamp); + assertTrue(save.isModified(f1)); + } + private File createFile(String string) throws IOException { trash.mkdirs(); File f = File.createTempFile(string, "tdat", trash); 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 d6b5fe57e..09e15938e 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 @@ -80,6 +80,8 @@ public class FileSnapshot { */ public static final long UNKNOWN_SIZE = -1; + private static final Object MISSING_FILEKEY = new Object(); + /** * A FileSnapshot that is considered to always be modified. *

@@ -88,7 +90,7 @@ public class FileSnapshot { * snapshot contains only invalid status information. */ public static final FileSnapshot DIRTY = new FileSnapshot(-1, -1, - UNKNOWN_SIZE, Duration.ZERO); + UNKNOWN_SIZE, Duration.ZERO, MISSING_FILEKEY); /** * A FileSnapshot that is clean if the file does not exist. @@ -98,7 +100,7 @@ public class FileSnapshot { * path does not exist. */ public static final FileSnapshot MISSING_FILE = new FileSnapshot(0, 0, 0, - Duration.ZERO) { + Duration.ZERO, MISSING_FILEKEY) { @Override public boolean isModified(File path) { return FS.DETECTED.exists(path); @@ -119,17 +121,26 @@ public static FileSnapshot save(File path) { long read = System.currentTimeMillis(); long modified; long size; + Object fileKey = null; Duration fsTimerResolution = FS .getFsTimerResolution(path.toPath().getParent()); try { BasicFileAttributes fileAttributes = FS.DETECTED.fileAttributes(path); modified = fileAttributes.lastModifiedTime().toMillis(); size = fileAttributes.size(); + fileKey = getFileKey(fileAttributes); } catch (IOException e) { modified = path.lastModified(); size = path.length(); + fileKey = MISSING_FILEKEY; } - return new FileSnapshot(read, modified, size, fsTimerResolution); + return new FileSnapshot(read, modified, size, fsTimerResolution, + fileKey); + } + + private static Object getFileKey(BasicFileAttributes fileAttributes) { + Object fileKey = fileAttributes.fileKey(); + return fileKey == null ? MISSING_FILEKEY : fileKey; } /** @@ -149,7 +160,8 @@ public static FileSnapshot save(File path) { */ public static FileSnapshot save(long modified) { final long read = System.currentTimeMillis(); - return new FileSnapshot(read, modified, -1, Duration.ZERO); + return new FileSnapshot(read, modified, -1, Duration.ZERO, + MISSING_FILEKEY); } /** Last observed modification time of the path. */ @@ -169,13 +181,20 @@ public static FileSnapshot save(long modified) { /** measured filesystem timestamp resolution */ private Duration fsTimestampResolution; + /** + * Object that uniquely identifies the given file, or {@code + * null} if a file key is not available + */ + private Object fileKey; + private FileSnapshot(long read, long modified, long size, - @NonNull Duration fsTimestampResolution) { + @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; } /** @@ -204,15 +223,20 @@ public long size() { public boolean isModified(File path) { long currLastModified; long currSize; + Object currFileKey; try { BasicFileAttributes fileAttributes = FS.DETECTED.fileAttributes(path); currLastModified = fileAttributes.lastModifiedTime().toMillis(); currSize = fileAttributes.size(); + currFileKey = getFileKey(fileAttributes); } catch (IOException e) { currLastModified = path.lastModified(); currSize = path.length(); + currFileKey = MISSING_FILEKEY; } - return (currSize != UNKNOWN_SIZE && currSize != size) || isModified(currLastModified); + return isSizeChanged(currSize) + || isFileKeyChanged(currFileKey) + || isModified(currLastModified); } /** @@ -252,7 +276,8 @@ public void setClean(FileSnapshot other) { * @return true if the two snapshots share the same information. */ public boolean equals(FileSnapshot other) { - return lastModified == other.lastModified && size == other.size; + return lastModified == other.lastModified && size == other.size + && Objects.equals(fileKey, other.fileKey); } /** {@inheritDoc} */ @@ -274,7 +299,8 @@ public boolean equals(Object obj) { /** {@inheritDoc} */ @Override public int hashCode() { - return Objects.hash(Long.valueOf(lastModified), Long.valueOf(size)); + return Objects.hash(Long.valueOf(lastModified), Long.valueOf(size), + fileKey); } /** {@inheritDoc} */ @@ -291,7 +317,7 @@ public String toString() { Locale.US); return "FileSnapshot[modified: " + f.format(new Date(lastModified)) + ", read: " + f.format(new Date(lastRead)) + ", size:" + size - + "]"; + + ", fileKey: " + fileKey + "]"; } private boolean notRacyClean(long read) { @@ -327,4 +353,12 @@ private boolean isModified(long currLastModified) { // return true; } + + private boolean isFileKeyChanged(Object currFileKey) { + return currFileKey != MISSING_FILEKEY && !currFileKey.equals(fileKey); + } + + private boolean isSizeChanged(long currSize) { + return currSize != UNKNOWN_SIZE && currSize != size; + } }