Include size when comparing FileSnapshot

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).

Read and consider file size also, so that differing file size can help
to more accurately detect file changes without reading the file content.
Use bulk read to avoid multiple stat calls to retrieve file attributes.

Change-Id: I974288fff78ac78c52245d9218b5639603f67a46
Signed-off-by: Luca Milanesio <luca.milanesio@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

5 files changed, 107 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 902416bdff..07b1bc789d 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.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -99,6 +100,27 @@ public class FileSnapshotTest {
 	}
 
 	/**
+	 * Check that file is modified by looking at its size.
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void tesIsModifiedBySameLastModifiedAndDifferentSize() throws Exception {
+		File f1 = createFile("foo", "lorem".getBytes());
+		File f2 = createFile("bar", "lorem ipsum".getBytes());
+
+		f1.setLastModified(f2.lastModified()); // Make sure f1 and f2 have the same lastModified
+		FileSnapshot save = FileSnapshot.save(f1);
+
+		// Make sure that the modified and read timestamps are far enough, so that
+		// check is done by size
+		Thread.sleep(3000L);
+
+		assertEquals(save.lastModified(), f2.lastModified());
+		assertTrue(save.isModified(f2));
+	}
+
+	/**
 	 * Create a file, wait long enough and verify that it has not been modified.
 	 * 3.5 seconds mean any difference between file system timestamp and system
 	 * clock should be significant.
@@ -152,10 +174,22 @@ public class FileSnapshotTest {
 		return f;
 	}
 
+	private File createFile(String string, byte[] content) throws IOException {
+		File f = createFile(string);
+		append(f, content);
+		return f;
+	}
+
 	private static void append(File f, byte b) throws IOException {
+		append(f, new byte[] { b });
+	}
+
+	private static void append(File f, byte[] content) throws IOException {
 		FileOutputStream os = new FileOutputStream(f, true);
 		try {
-			os.write(b);
+			for (byte b : content) {
+				os.write(b);
+			}
 		} finally {
 			os.close();
 		}
diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters
index d259fc85f0..fe83211ebc 100644
--- a/org.eclipse.jgit/.settings/.api_filters
+++ b/org.eclipse.jgit/.settings/.api_filters
@@ -22,4 +22,12 @@
             </message_arguments>
         </filter>
     </resource>
+    <resource path="src/org/eclipse/jgit/util/FS.java" type="org.eclipse.jgit.util.FS">
+        <filter id="1142947843">
+            <message_arguments>
+                <message_argument value="4.5.6"/>
+                <message_argument value="fileAttributes(File)"/>
+            </message_arguments>
+        </filter>
+    </resource>
 </component>
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 8926d79306..fbd1dbf696 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
@@ -45,6 +45,7 @@ package org.eclipse.jgit.internal.storage.file;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
 import java.util.Date;
@@ -70,13 +71,20 @@ import org.eclipse.jgit.util.FS;
  */
 public class FileSnapshot {
 	/**
+	 * An unknown file size.
+	 *
+	 * This value is used when a comparison needs to happen purely on the lastUpdate.
+	 */
+	public static final long UNKNOWN_SIZE = -1;
+
+	/**
 	 * A FileSnapshot that is considered to always be modified.
 	 * <p>
 	 * This instance is useful for application code that wants to lazily read a
 	 * file, but only after {@link #isModified(File)} gets invoked. The returned
 	 * snapshot contains only invalid status information.
 	 */
-	public static final FileSnapshot DIRTY = new FileSnapshot(-1, -1);
+	public static final FileSnapshot DIRTY = new FileSnapshot(-1, -1, UNKNOWN_SIZE);
 
 	/**
 	 * A FileSnapshot that is clean if the file does not exist.
@@ -85,7 +93,7 @@ public class FileSnapshot {
 	 * file to be clean. {@link #isModified(File)} will return false if the file
 	 * path does not exist.
 	 */
-	public static final FileSnapshot MISSING_FILE = new FileSnapshot(0, 0) {
+	public static final FileSnapshot MISSING_FILE = new FileSnapshot(0, 0, 0) {
 		@Override
 		public boolean isModified(File path) {
 			return FS.DETECTED.exists(path);
@@ -105,12 +113,16 @@ public class FileSnapshot {
 	public static FileSnapshot save(File path) {
 		long read = System.currentTimeMillis();
 		long modified;
+		long size;
 		try {
-			modified = FS.DETECTED.lastModified(path);
+			BasicFileAttributes fileAttributes = FS.DETECTED.fileAttributes(path);
+			modified = fileAttributes.lastModifiedTime().toMillis();
+			size = fileAttributes.size();
 		} catch (IOException e) {
 			modified = path.lastModified();
+			size = path.length();
 		}
-		return new FileSnapshot(read, modified);
+		return new FileSnapshot(read, modified, size);
 	}
 
 	/**
@@ -126,7 +138,7 @@ public class FileSnapshot {
 	 */
 	public static FileSnapshot save(long modified) {
 		final long read = System.currentTimeMillis();
-		return new FileSnapshot(read, modified);
+		return new FileSnapshot(read, modified, -1);
 	}
 
 	/** Last observed modification time of the path. */
@@ -138,10 +150,16 @@ public class FileSnapshot {
 	/** True once {@link #lastRead} is far later than {@link #lastModified}. */
 	private boolean cannotBeRacilyClean;
 
-	private FileSnapshot(long read, long modified) {
+	/** Underlying file-system size in bytes.
+	 *
+	 * When set to {@link #UNKNOWN_SIZE} the size is not considered for modification checks. */
+	private final long size;
+
+	private FileSnapshot(long read, long modified, long size) {
 		this.lastRead = read;
 		this.lastModified = modified;
 		this.cannotBeRacilyClean = notRacyClean(read);
+		this.size = size;
 	}
 
 	/**
@@ -152,6 +170,13 @@ public class FileSnapshot {
 	}
 
 	/**
+	 * @return file size in bytes of last snapshot update
+	 */
+	public long size() {
+		return size;
+	}
+
+	/**
 	 * Check if the path may have been modified since the snapshot was saved.
 	 *
 	 * @param path
@@ -160,12 +185,16 @@ public class FileSnapshot {
 	 */
 	public boolean isModified(File path) {
 		long currLastModified;
+		long currSize;
 		try {
-			currLastModified = FS.DETECTED.lastModified(path);
+			BasicFileAttributes fileAttributes = FS.DETECTED.fileAttributes(path);
+			currLastModified = fileAttributes.lastModifiedTime().toMillis();
+			currSize = fileAttributes.size();
 		} catch (IOException e) {
 			currLastModified = path.lastModified();
+			currSize = path.length();
 		}
-		return isModified(currLastModified);
+		return (currSize != UNKNOWN_SIZE && currSize != size) || isModified(currLastModified);
 	}
 
 	/**
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
index e1fd1cb889..530b1d4bc2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -52,6 +52,7 @@ import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.nio.charset.Charset;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.text.MessageFormat;
@@ -418,6 +419,19 @@ public abstract class FS {
 	public abstract boolean retryFailedLockFileCommit();
 
 	/**
+	 * Return all the attributes of a file, without following symbolic links.
+	 *
+	 * @param file
+	 * @return {@link BasicFileAttributes} of the file
+	 * @throws IOException in case of any I/O errors accessing the file
+	 *
+	 * @since 4.5.6
+	 */
+	public BasicFileAttributes fileAttributes(File file) throws IOException {
+		return FileUtils.fileAttributes(file);
+	}
+
+	/**
 	 * Determine the user's home directory (location where preferences are).
 	 *
 	 * @return the user's home directory; null if the user does not have one.
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
index aa101f73f9..88b1f9ea01 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
@@ -565,6 +565,19 @@ public class FileUtils {
 	}
 
 	/**
+	 * Return all the attributes of a file, without following symbolic links.
+	 *
+	 * @param file
+	 * @return {@link BasicFileAttributes} of the file
+	 * @throws IOException in case of any I/O errors accessing the file
+	 *
+	 * @since 4.5.6
+	 */
+	static BasicFileAttributes fileAttributes(File file) throws IOException {
+		return Files.readAttributes(file.toPath(), BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
+	}
+
+	/**
 	 * @param file
 	 * @param time
 	 * @throws IOException