Better handling of directory mod times.

Mirror is fundamentally based on mod times, i.e. latest mod time wins.

This works great for files, but directories are a bit different, because
any new/changed file in a directory, updates the directory's own mod
time to an OS-generated value.

In general we don't want to bother with keeping these in-sync across
local/remote, so now we explicitly ignore this case.

Deletions also don't work well with mod times, b/c a deletion doesn't
really have a time in the file system anymore; previously we added the
old mod time + 1 second immediately when the delete happened, but this
could result in an ignored write, hwen our "now + 1 second" hueristic
was artificially ahead of an actual now + < 1 second write on the
just-deleted file.

We still need to fudge with mod times on "deleted then restored via
mv" files, to ensure their still-in-the-past-b/c-it-was-restored mod
time "wins" over the deletion mark, so we recognize when that is
happening and bump the restored file's mod time ahead by 1 second.

src/main/java/mirror/UpdateTree.java

@@ -170,8 +170,6 @@ public enum NodeType {
     File, Directory, Symlink
   }
 
-  ;
-
   /** Either a directory or file within the tree. */
   public class Node {
     private final Node parent;
@@ -209,15 +207,25 @@ Update getLocal() {
     }
 
     void setLocal(Update local) {
-      // The best we can do for guessing the mod time of deletions
-      // is to take the old, known mod time and just tick 1
+      // Deleted files don't have a modtime, so keep the previous mod time
       if (local != null && this.local != null && local.getDelete() && local.getModTime() == 0L) {
-        int tick = this.local.getDelete() ? 0 : minimumMillisPrecision;
-        local = local.toBuilder().setModTime(this.local.getModTime() + tick).build();
+        local = local.toBuilder().setModTime(this.local.getModTime()).build();
+      }
+      // If we're a directory, every write to a child directory bumps our modtime. This can cause us to drift
+      // ahead of the remote, so we purposefully pin ourselves to the prior modtime. I.e. we only want directory
+      // modtimes to advance on explicit actions like delete/re-create.
+      if (local != null && this.local != null && local.getDirectory() && this.local.getDirectory()) {
+        local = local.toBuilder().setModTime(this.local.getModTime()).build();
       }
-      // If we can tell the incoming local update is meant to re-create the previous
-      // delete marker, ensure that we bump it to a most-delete marker modtime. E.g.
-      // sometimes the restored file will keep the pre-delete marker timestamp.
+      // Restored files that were marked deleted but restored w/the same modtime need a newer
+      // modtime to force both the local & remote diff logic to know that this restored version wins.
+      if (local != null && this.local != null && !local.getDelete() && this.local.getDelete() && local.getModTime() <= this.local.getModTime()) {
+        // TODO Should we write this modtime bump back to our local file system?
+        local = local.toBuilder().setModTime(this.local.getModTime() + minimumMillisPrecision).build();
+      }
+      // If we can tell the incoming local update is meant to re-create the previous delete marker,
+      // ensure that we bump it to a most-delete marker modtime. E.g. sometimes the restored file will
+      // keep the pre-delete marker timestamp (like when it is `mv`'d instead of created as a new file).
       if (local != null && this.local != null && this.local.getDelete() && this.local.getModTime() > local.getModTime()) {
         // Should we update the local file system? Probably, but currently that isn't the UpdateTree's job
         local = local.toBuilder().setModTime(this.local.getModTime() + minimumMillisPrecision).build();
@@ -258,8 +266,25 @@ boolean isNewer(Update a, Update b) {
       if (a == null) {
         return false;
       }
-      boolean isNewer = b == null || (sanityCheckTimestamp(a.getModTime()) > sanityCheckTimestamp(b.getModTime()));
+
+      long aTime = sanityCheckTimestamp(a.getModTime());
+      long bTime = b == null ? 0 : sanityCheckTimestamp(b.getModTime());
+
+      // For deletes, we keep the same modtime as the last file, so we have to combine that with the deleted flag.
+      boolean aDeleteWins = aTime == bTime && a.getDelete() && b != null && !b.getDelete();
+      boolean bDeleteWins = aTime == bTime && !a.getDelete() && b != null && b.getDelete();
+      if (aDeleteWins) {
+        return true;
+      } else if (bDeleteWins) {
+        return false;
+      }
+
+      boolean isNewer = aTime > bTime || b == null;
+      // Don't bother sending deletes to a remote that is already deleted/doesn't exist
       boolean isNoopDelete = a.getDelete() && (b == null || b.getDelete());
+      // Anytime we write to a file like `foo/bar/zaz.txt` (like from an incoming update),
+      // the modtimes of each parent directory is updated automatically by the file system,
+      // which we pick up as local changes, but we don't really need to sync these back over.
       boolean isDirModtimeChange = !a.getDelete() && UpdateTree.isDirectory(a) && b != null && UpdateTree.isDirectory(b) && !b.getDelete();
       return isNewer && !isNoopDelete && !isDirModtimeChange;
     }
@@ -313,7 +338,6 @@ boolean isDirectory() {
       return local != null ? UpdateTree.isDirectory(local) : remote != null ? UpdateTree.isDirectory(remote) : false;
     }
 
-    /** @param p should be a relative path, e.g. a/b/c.txt. */
     boolean shouldIgnore() {
       if (shouldIgnore != null) {
         return shouldIgnore;
@@ -418,7 +442,7 @@ private long sanityCheckTimestamp(long millis) {
     return (millis < minimumMillisPrecision) ? millis : millis / minimumMillisPrecision * minimumMillisPrecision;
   }
 
-  /** Visits nodes in the tree, in breadth-first order, continuing if {@visitor} returns true. */
+  /** Visits nodes in the tree, in breadth-first order, continuing if {@param visitor} returns true. */
   private static void visit(Node start, Predicate<Node> visitor) {
     Queue<Node> queue = new LinkedBlockingQueue<Node>();
     queue.add(start);

src/test/java/mirror/UpdateTreeTest.java

@@ -97,7 +97,7 @@ public void deleteFileMarksTheNodeAsDeleted() {
     root.addLocal(Update.newBuilder().setPath("foo.txt").setDelete(true).build());
     assertThat(root.getChildren().size(), is(1));
     assertThat(root.getChildren().get(0).getLocal().getDelete(), is(true));
-    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1001L));
+    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1L));
   }
 
   @Test
@@ -143,15 +143,21 @@ public void deleteThenRestoreFile() {
     assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1002L));
   }
 
-
   @Test
   public void deleteFileTwiceDoesNotRetickModTime() {
     root.addLocal(Update.newBuilder().setPath("foo.txt").setModTime(1L).build());
     root.addLocal(Update.newBuilder().setPath("foo.txt").setDelete(true).build());
     assertThat(root.getChildren().size(), is(1));
-    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1001L));
+    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1L));
     root.addLocal(Update.newBuilder().setPath("foo.txt").setDelete(true).build());
-    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1001L));
+    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1L));
+  }
+
+  @Test
+  public void ignoreFileSystemDirectoryModTimeTicks() {
+    root.addLocal(Update.newBuilder().setPath("foo").setDirectory(true).setModTime(1L).build());
+    root.addLocal(Update.newBuilder().setPath("foo").setDirectory(true).setModTime(2L).build());
+    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1L));
   }
 
   @Test(expected = IllegalArgumentException.class)
@@ -282,6 +288,19 @@ public void isNewerForDeletedFile() {
     assertThat(root.getChildren().get(0).isLocalNewer(), is(true));
   }
 
+  @Test
+  public void isNewerForRestoredFiles() {
+    // given we'd marked foo as deleted
+    root.addLocal(Update.newBuilder().setPath("foo").setDelete(true).setModTime(1).build());
+    root.addRemote(Update.newBuilder().setPath("foo").setDelete(true).setModTime(1).build());
+    // when a new non-delete comes in with the same modtime (i.e. a `mv` of the old file)
+    root.addLocal(Update.newBuilder().setPath("foo").setModTime(1).build());
+    // then we've artificially ticked the modtime ahead so we can recognize it as newer
+    assertThat(root.getChildren().get(0).getLocal().getModTime(), is(1001L));
+    assertThat(root.getChildren().get(0).isLocalNewer(), is(true));
+    assertThat(root.getChildren().get(0).isRemoteNewer(), is(false));
+  }
+
   @Test
   public void isNewerForCorruptModTimes() {
     long now = System.currentTimeMillis();