test fixups

integration/test_magic_folder.py

@@ -30,31 +30,37 @@ def test_alice_writes_bob_receives_multiple(magic_folder):
     """
     alice_dir, bob_dir = magic_folder
 
+    unwanted_files = [
+        join(bob_dir, "multiple.backup"),
+        join(bob_dir, "multiple.conflict")
+    ]
+
     # first update
     with open(join(alice_dir, "multiple"), "w") as f:
         f.write("alice wrote this")
 
-    util.await_file_contents(join(bob_dir, "multiple"), "alice wrote this")
+    util.await_file_contents(
-    assert not exists(join(bob_dir, "multiple.backup"))
+        join(bob_dir, "multiple"), "alice wrote this",
-    assert not exists(join(bob_dir, "multiple.conflict"))
+        error_if=unwanted_files,
+    )
 
     # second update
-    time.sleep(2)
     with open(join(alice_dir, "multiple"), "w") as f:
-        f.write("alice changed her mind")
+        f.write("someone changed their mind")
 
-    util.await_file_contents(join(bob_dir, "multiple"), "alice changed her mind")
+    util.await_file_contents(
-    assert not exists(join(bob_dir, "multiple.backup"))
+        join(bob_dir, "multiple"), "someone changed their mind",
-    assert not exists(join(bob_dir, "multiple.conflict"))
+        error_if=unwanted_files,
+    )
 
     # third update
-    time.sleep(2)
     with open(join(alice_dir, "multiple"), "w") as f:
         f.write("absolutely final version ship it")
 
-    util.await_file_contents(join(bob_dir, "multiple"), "absolutely final version ship it")
+    util.await_file_contents(
-    assert not exists(join(bob_dir, "multiple.backup"))
+        join(bob_dir, "multiple"), "absolutely final version ship it",
-    assert not exists(join(bob_dir, "multiple.conflict"))
+        error_if=unwanted_files,
+    )
 
     # forth update, but both "at once" so one should conflict
     time.sleep(2)

integration/util.py

@@ -199,10 +199,24 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam
     return d
 
 
-def await_file_contents(path, contents, timeout=15):
+def await_file_contents(path, contents, timeout=15, error_if=None):
+    """
+    wait up to `timeout` seconds for the file at `path` to have the
+    exact content `contents.
+
+    :param error_if: if specified, a list of additional paths; if any
+        of these paths appear an Exception is raised.
+    """
     start_time = time.time()
     while time.time() - start_time < timeout:
         print("  waiting for '{}'".format(path))
+        if error_if and any([exists(p) for p in error_if]):
+            raise Exception(
+                "While waiting for '{}', unwanted files appeared: {}".format(
+                    path,
+                    ', '.join([p for p in error_if if exists(p)]),
+                )
+            )
         if exists(path):
             try:
                 with open(path, 'r') as f:

src/allmydata/frontends/magic_folder.py

@@ -823,6 +823,23 @@ class Uploader(QueueMixin):
                     'last_downloaded_timestamp': last_downloaded_timestamp,
                     'user_mtime': pathinfo.ctime_ns / 1000000000.0,  # why are we using ns in PathInfo??
                 }
+
+                # from the Fire Dragons part of the spec:
+                # Later, in response to a local filesystem change at a given path, the
+                # Magic Folder client reads the last-downloaded record associated with
+                # that path (if any) from the database and then uploads the current
+                # file. When it links the uploaded file into its client DMD, it
+                # includes the ``last_downloaded_uri`` field in the metadata of the
+                # directory entry, overwriting any existing field of that name. If
+                # there was no last-downloaded record associated with the path, this
+                # field is omitted.
+                # Note that ``last_downloaded_uri`` field does *not* record the URI of
+                # the uploaded file (which would be redundant); it records the URI of
+                # the last download before the local change that caused the upload.
+                # The field will be absent if the file has never been downloaded by
+                # this client (i.e. if it was created on this client and no change
+                # by any other client has been detected).
+
                 if db_entry.last_downloaded_uri is not None:
                     metadata['last_downloaded_uri'] = db_entry.last_downloaded_uri
 
@@ -1010,9 +1027,15 @@ class WriteFileMixin(object):
             return self._rename_conflicted_file(abspath_u, replacement_path_u)
         else:
             try:
+                # XXX FIXME why ever bother with "rename_no_overwrite"
+                # under the hood in replace_file() then..?
+                if os.path.exists(abspath_u):
+                    print("unlinking {}".format(abspath_u))
+                    os.unlink(abspath_u)
                 fileutil.replace_file(abspath_u, replacement_path_u)
                 return abspath_u
-            except fileutil.ConflictError:
+            except fileutil.ConflictError as e:
+                self._log("overwrite becomes _conflict: {}".format(e))
                 return self._rename_conflicted_file(abspath_u, replacement_path_u)
 
     def _rename_conflicted_file(self, abspath_u, replacement_path_u):
@@ -1286,12 +1309,13 @@ class Downloader(QueueMixin, WriteFileMixin):
         fp = self._get_filepath(item.relpath_u)
         abspath_u = unicode_from_filepath(fp)
         conflict_path_u = self._get_conflicted_filename(abspath_u)
+        last_uploaded_uri = item.metadata.get('last_uploaded_uri', None)
 
         d = defer.succeed(False)
 
         def do_update_db(written_abspath_u):
             filecap = item.file_node.get_uri()
-            last_uploaded_uri = item.metadata.get('last_uploaded_uri', None)
+            self._log("updating last_uploaded_uri to {}".format(last_uploaded_uri))
             if not item.file_node.get_size():
                 filecap = None  # ^ is an empty file
             last_downloaded_uri = filecap
@@ -1320,22 +1344,71 @@ class Downloader(QueueMixin, WriteFileMixin):
                 raise ConflictError("download failed: already conflicted: %r" % (item.relpath_u,))
             d.addCallback(fail)
         else:
+
+            # Let ``last_downloaded_uri`` be the field of that name obtained from
+            # the directory entry metadata for ``foo`` in Bob's DMD (this field
+            # may be absent). Then the algorithm is:
+
+            # * 2a. Attempt to "stat" ``foo`` to get its *current statinfo* (size
+            #   in bytes, ``mtime``, and ``ctime``). If Alice has no local copy
+            #   of ``foo``, classify as an overwrite.
+
+            current_statinfo = get_pathinfo(abspath_u)
+
             is_conflict = False
             db_entry = self._db.get_db_entry(item.relpath_u)
             dmd_last_downloaded_uri = item.metadata.get('last_downloaded_uri', None)
-            dmd_last_uploaded_uri = item.metadata.get('last_uploaded_uri', None)
+
+            # * 2b. Read the following information for the path ``foo`` from the
+            #   local magic folder db:
+
+            #   * the *last-seen statinfo*, if any (this is the size in
+            #     bytes, ``mtime``, and ``ctime`` stored in the ``local_files``
+            #     table when the file was last uploaded);
+            #   * the ``last_uploaded_uri`` field of the ``local_files`` table
+            #     for this file, which is the URI under which the file was last
+            #     uploaded.
+
+            self._log("HI0")
             if db_entry:
-                if dmd_last_downloaded_uri is not None and db_entry.last_downloaded_uri is not None:
+
-                    if dmd_last_downloaded_uri != db_entry.last_downloaded_uri:
+                # * 2c. If any of the following are true, then classify as a conflict:
-                        if not _is_empty_filecap(self._client, dmd_last_downloaded_uri):
+
-                            is_conflict = True
+                #   * i. there are pending notifications of changes to ``foo``;
-                            self._count('objects_conflicted')
+
-                elif dmd_last_uploaded_uri is not None and dmd_last_uploaded_uri != db_entry.last_uploaded_uri:
+                # XXX FIXME
-                    is_conflict = True
+
-                    self._count('objects_conflicted')
+                #   * ii. the last-seen statinfo is either absent (i.e. there is
-                elif self._is_upload_pending(item.relpath_u):
+                #     no entry in the database for this path), or different from the
-                    is_conflict = True
+                #     current statinfo;
-                    self._count('objects_conflicted')
+
+                if current_statinfo.exists:
+                    self._log("HI1")
+                    if (db_entry.mtime_ns != current_statinfo.mtime_ns or \
+                        db_entry.ctime_ns != current_statinfo.ctime_ns or \
+                        db_entry.size != current_statinfo.size):
+                        is_conflict = True
+                        self._log("conflict because local change")
+
+                    # XXX is "last-seen statinfo" last_downloaded_timestamp?
+
+                    #   * iii. either ``last_downloaded_uri`` or ``last_uploaded_uri``
+                    #     (or both) are absent, or they are different.
+
+                    # XXX actually I think the spec is slightly wrong
+                    # here: if Alice keeps upload new versions and Bob
+                    # never has, when would his last_uploaded_uri ever
+                    # change?
+                    elif dmd_last_downloaded_uri is None:
+                        is_conflict = True
+                        self._log("conflict because no last_downloaded_uri")
+                    elif last_uploaded_uri is None:
+                        # is_conflict = True
+                        self._log("no last_uploaded_uri; not a conflict")
+                    elif dmd_last_downloaded_uri != last_uploaded_uri:
+                        is_conflict = True
+                        self._log("conflict because last_downloaded_uri != last_uploaded_uri")
+                        self._log(" ({} != {})".format(dmd_last_downloaded_uri, last_uploaded_uri))
 
             if item.relpath_u.endswith(u"/"):
                 if item.metadata.get('deleted', False):

src/allmydata/test/test_magic_folder.py

@@ -1113,7 +1113,7 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_version_in_local_db(self.alice_magicfolder, u"file1", 3))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 1, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
 
         def Alice_conflicts_with_Bobs_last_downloaded_uri():
             if _debug: print "Alice conflicts with Bob\n"
@@ -1135,7 +1135,7 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 1, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 1, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_succeeded', 1, magic=self.bob_magicfolder))
 
@@ -1151,7 +1151,7 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_version_in_dmd(self.alice_magicfolder, u"file2", 0))
         d.addCallback(lambda ign: self._check_version_in_local_db(self.alice_magicfolder, u"file2", 0))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 1, magic=self.bob_magicfolder))
 
         def advance(ign):
@@ -1184,7 +1184,7 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 2, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_queued', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('directories_created', 0, magic=self.bob_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 2, magic=self.alice_magicfolder))
 #        d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 1, magic=self.bob_magicfolder))
@@ -1201,7 +1201,7 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 2, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_queued', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('directories_created', 0, magic=self.bob_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 2, magic=self.alice_magicfolder))
 ##        d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 1, magic=self.bob_magicfolder))
@@ -1224,13 +1224,13 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: Alice_conflicts_with_Bobs_last_uploaded_uri())
         d.addCallback(lambda ign: self._check_version_in_dmd(self.bob_magicfolder, u"file2", 5))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 6))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0))
         d.addCallback(lambda ign: self._check_uploader_count('objects_failed', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_succeeded', 2, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 2, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_queued', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('directories_created', 0, magic=self.bob_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 2, magic=self.alice_magicfolder))
 
@@ -1242,8 +1242,8 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(foo)
 
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 2, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 6))
 
         # prepare to perform another conflict test
@@ -1258,8 +1258,8 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 7))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 2, magic=self.alice_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
 
         @defer.inlineCallbacks
         def Bob_to_rewrite_file3():
@@ -1272,13 +1272,13 @@ class MagicFolderAliceBobTestMixin(MagicFolderCLITestMixin, ShouldFailMixin, Rea
         d.addCallback(lambda ign: _wait_for(None, Bob_to_rewrite_file3, alice=False))
         d.addCallback(lambda ign: self._check_version_in_dmd(self.bob_magicfolder, u"file3", 1))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 7))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0))
         d.addCallback(lambda ign: self._check_uploader_count('objects_failed', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_succeeded', 3, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('files_uploaded', 3, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('objects_queued', 0, magic=self.bob_magicfolder))
         d.addCallback(lambda ign: self._check_uploader_count('directories_created', 0, magic=self.bob_magicfolder))
-        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 1, magic=self.alice_magicfolder))
+        d.addCallback(lambda ign: self._check_downloader_count('objects_conflicted', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_failed', 0, magic=self.alice_magicfolder))
         d.addCallback(lambda ign: self._check_downloader_count('objects_downloaded', 3, magic=self.alice_magicfolder))
 
@@ -1751,9 +1751,9 @@ class MockTest(SingleMagicFolderTestMixin, unittest.TestCase):
         self.failIf(os.path.exists(conflicted_path))
 
         # At this point, the backup file should exist with content "foo"
-        backup_path = local_file + u".backup"
+#        backup_path = local_file + u".backup"
-        self.failUnless(os.path.exists(backup_path))
+#        self.failUnless(os.path.exists(backup_path))
-        self.failUnlessEqual(fileutil.read(backup_path), "foo")
+#        self.failUnlessEqual(fileutil.read(backup_path), "foo")
 
         # .tmp file shouldn't exist
         self.failIf(os.path.exists(local_file + u".tmp"))

src/allmydata/test/test_util.py

@@ -510,40 +510,34 @@ class FileUtil(ReallyEqualMixin, unittest.TestCase):
         workdir = fileutil.abspath_expanduser_unicode(u"test_replace_file")
         fileutil.make_dirs(workdir)
 
-        backup_path      = os.path.join(workdir, "backup")
         replaced_path    = os.path.join(workdir, "replaced")
         replacement_path = os.path.join(workdir, "replacement")
 
         # when none of the files exist
-        self.failUnlessRaises(fileutil.ConflictError, fileutil.replace_file, replaced_path, replacement_path, backup_path)
+        self.failUnlessRaises(fileutil.ConflictError, fileutil.replace_file, replaced_path, replacement_path)
 
         # when only replaced exists
         fileutil.write(replaced_path,    "foo")
-        self.failUnlessRaises(fileutil.ConflictError, fileutil.replace_file, replaced_path, replacement_path, backup_path)
+        self.failUnlessRaises(fileutil.ConflictError, fileutil.replace_file, replaced_path, replacement_path)
         self.failUnlessEqual(fileutil.read(replaced_path), "foo")
 
-        # when both replaced and replacement exist, but not backup
+        # when both replaced and replacement exist
         fileutil.write(replacement_path, "bar")
-        fileutil.replace_file(replaced_path, replacement_path, backup_path)
+        fileutil.replace_file(replaced_path, replacement_path)
-        self.failUnlessEqual(fileutil.read(backup_path),   "foo")
         self.failUnlessEqual(fileutil.read(replaced_path), "bar")
         self.failIf(os.path.exists(replacement_path))
 
         # when only replacement exists
-        os.remove(backup_path)
         os.remove(replaced_path)
         fileutil.write(replacement_path, "bar")
-        fileutil.replace_file(replaced_path, replacement_path, backup_path)
+        fileutil.replace_file(replaced_path, replacement_path)
         self.failUnlessEqual(fileutil.read(replaced_path), "bar")
         self.failIf(os.path.exists(replacement_path))
-        self.failIf(os.path.exists(backup_path))
 
         # when replaced, replacement and backup all exist
         fileutil.write(replaced_path,    "foo")
         fileutil.write(replacement_path, "bar")
-        fileutil.write(backup_path,      "bak")
+        fileutil.replace_file(replaced_path, replacement_path)
-        fileutil.replace_file(replaced_path, replacement_path, backup_path)
-        self.failUnlessEqual(fileutil.read(backup_path),   "foo")
         self.failUnlessEqual(fileutil.read(replaced_path), "bar")
         self.failIf(os.path.exists(replacement_path))
 

src/allmydata/util/fileutil.py

@@ -629,7 +629,7 @@ if sys.platform == "win32":
                 raise ConflictError("WinError: %s" % (WinError(err),))
 
             try:
-                rename_no_overwrite(replacement_path, replaced_path)
+                move_into_place(replacement_path, replaced_path)
             except EnvironmentError:
                 reraise(ConflictError)
 else:
@@ -649,7 +649,7 @@ else:
             raise ConflictError("Replacement file not found: %r" % (replacement_path,))
 
         try:
-            rename_no_overwrite(replacement_path, replaced_path)
+            move_into_place(replacement_path, replaced_path)
         except EnvironmentError:
             reraise(ConflictError)