From 2d31fa2f9ac5027b5c80534e48a63bc969842d91 Mon Sep 17 00:00:00 2001 From: Stephen Haberman Date: Sun, 5 Jan 2020 15:00:11 -0600 Subject: [PATCH] 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 | 48 ++++++++++++++++++------ src/test/java/mirror/UpdateTreeTest.java | 27 +++++++++++-- 2 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/main/java/mirror/UpdateTree.java b/src/main/java/mirror/UpdateTree.java index f3206f44..6b338888 100644 --- a/src/main/java/mirror/UpdateTree.java +++ b/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 visitor) { Queue queue = new LinkedBlockingQueue(); queue.add(start); diff --git a/src/test/java/mirror/UpdateTreeTest.java b/src/test/java/mirror/UpdateTreeTest.java index 6aa1b844..9a6fd0d5 100644 --- a/src/test/java/mirror/UpdateTreeTest.java +++ b/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();