FileBasedConfig: Use FileSnapshot for isOutdated()

Relying only on the last modified time for a file can be tricky.
The "racy git" problem may cause some modifications to be missed.

Use the new FileSnapshot code to track when a configuration file
has been modified, and needs to be reloaded in memory.

Change-Id: Ib6312fdd3b2403eee5af3f8ae711294b0e5f9035
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
index da1f3af603359eb56e0469ff1553eb04cd0244cb..212646b11db0c4158351a256d6b2b66c1dfb6a61 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java
@@ -58,6 +58,7 @@ import org.eclipse.jgit.JGitText;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.IO;
@@ -68,8 +69,9 @@ import org.eclipse.jgit.util.RawParseUtils;
  */
 public class FileBasedConfig extends StoredConfig {
        private final File configFile;
-       private volatile long lastModified;
        private final FS fs;
+       private volatile FileSnapshot snapshot;
+       private volatile ObjectId hash;
 
        /**
         * Create a configuration with no default fallback.
@@ -99,6 +101,8 @@ public class FileBasedConfig extends StoredConfig {
                super(base);
                configFile = cfgLocation;
                this.fs = fs;
+               this.snapshot = FileSnapshot.DIRTY;
+               this.hash = ObjectId.zeroId();
        }
 
        @Override
@@ -125,11 +129,24 @@ public class FileBasedConfig extends StoredConfig {
         */
        @Override
        public void load() throws IOException, ConfigInvalidException {
-               lastModified = getFile().lastModified();
+               final FileSnapshot oldSnapshot = snapshot;
+               final FileSnapshot newSnapshot = FileSnapshot.save(getFile());
                try {
-                       fromText(RawParseUtils.decode(IO.readFully(getFile())));
+                       final byte[] in = IO.readFully(getFile());
+                       final ObjectId newHash = hash(in);
+                       if (hash.equals(newHash)) {
+                               if (oldSnapshot.equals(newSnapshot))
+                                       oldSnapshot.setClean(newSnapshot);
+                               else
+                                       snapshot = newSnapshot;
+                       } else {
+                               fromText(RawParseUtils.decode(in));
+                               snapshot = newSnapshot;
+                               hash = newHash;
+                       }
                } catch (FileNotFoundException noFile) {
                        clear();
+                       snapshot = newSnapshot;
                } catch (IOException e) {
                        final IOException e2 = new IOException(MessageFormat.format(JGitText.get().cannotReadFile, getFile()));
                        e2.initCause(e);
@@ -157,18 +174,29 @@ public class FileBasedConfig extends StoredConfig {
                if (!lf.lock())
                        throw new IOException(MessageFormat.format(JGitText.get().cannotLockFile, getFile()));
                try {
-                       lf.setNeedStatInformation(true);
+                       lf.setNeedSnapshot(true);
                        lf.write(out);
                        if (!lf.commit())
                                throw new IOException(MessageFormat.format(JGitText.get().cannotCommitWriteTo, getFile()));
                } finally {
                        lf.unlock();
                }
-               lastModified = lf.getCommitLastModified();
+               snapshot = lf.getCommitSnapshot();
+               hash = hash(out);
                // notify the listeners
                fireConfigChangedEvent();
        }
 
+       @Override
+       public void clear() {
+               hash = hash(new byte[0]);
+               super.clear();
+       }
+
+       private static ObjectId hash(final byte[] rawText) {
+               return ObjectId.fromRaw(Constants.newMessageDigest().digest(rawText));
+       }
+
        @Override
        public String toString() {
                return getClass().getSimpleName() + "[" + getFile().getPath() + "]";
@@ -179,6 +207,6 @@ public class FileBasedConfig extends StoredConfig {
         * than the file on disk
         */
        public boolean isOutdated() {
-               return getFile().lastModified() != lastModified;
+               return snapshot.isModified(getFile());
        }
 }

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 c1ce449dcec3bbe1c0345b125e982dab926116d1..87ae57b8242453076bdcfe5967db410cb9c5f2e1 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java
@@ -104,6 +104,10 @@ public class FileSnapshot {
                this.cannotBeRacilyClean = notRacyClean(read);
        }
 
+       long lastModified() {
+               return lastModified;
+       }
+
        /**
         * Check if the path has been modified since the snapshot was saved.
         *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
index c1fb704a516b2181759404d0317ef8f980505351..52aa7eeb9db104b3cd843ce34d9c7256e84f72bf 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/LockFile.java
@@ -89,11 +89,11 @@ public class LockFile {
 
        private FileOutputStream os;
 
-       private boolean needStatInformation;
+       private boolean needSnapshot;
 
        private boolean fsync;
 
-       private long commitLastModified;
+       private FileSnapshot commitSnapshot;
 
        private final FS fs;
 
@@ -334,12 +334,24 @@ public class LockFile {
 
        /**
         * Request that {@link #commit()} remember modification time.
+        * <p>
+        * This is an alias for {@code setNeedSnapshot(true)}.
         *
         * @param on
         *            true if the commit method must remember the modification time.
         */
        public void setNeedStatInformation(final boolean on) {
-               needStatInformation = on;
+               setNeedSnapshot(on);
+       }
+
+       /**
+        * Request that {@link #commit()} remember the {@link FileSnapshot}.
+        *
+        * @param on
+        *            true if the commit method must remember the FileSnapshot.
+        */
+       public void setNeedSnapshot(final boolean on) {
+               needSnapshot = on;
        }
 
        /**
@@ -442,8 +454,8 @@ public class LockFile {
        }
 
        private void saveStatInformation() {
-               if (needStatInformation)
-                       commitLastModified = lck.lastModified();
+               if (needSnapshot)
+                       commitSnapshot = FileSnapshot.save(lck);
        }
 
        /**
@@ -452,7 +464,12 @@ public class LockFile {
         * @return modification time of the lock file right before we committed it.
         */
        public long getCommitLastModified() {
-               return commitLastModified;
+               return commitSnapshot.lastModified();
+       }
+
+       /** @return get the {@link FileSnapshot} just before commit. */
+       public FileSnapshot getCommitSnapshot() {
+               return commitSnapshot;
        }
 
        /**