PropagateDownload: Adjustments to skipping downloads #6153

Previously we required matching mtimes but that's actually
unnecessary when the question is about whether to skip the
download. We will still update the file's metadata.

Also, adjust behavior when the checksum is weak (Adler32):
in these cases we still depend on equal mtimes.

src/csync/csync_reconcile.cpp

@@ -63,18 +63,6 @@ static csync_file_stat_t *_csync_check_ignored(csync_s::FileMap *tree, const Byt
     }
 }
 
-/* Returns true if we're reasonably certain that hash equality
- * for the header means content equality.
- *
- * Cryptographic safety is not required - this is mainly
- * intended to rule out checksums like Adler32 that are not intended for
- * hashing and have high likelihood of collision with particular inputs.
- */
-static bool _csync_is_collision_safe_hash(const char *checksum_header)
-{
-    return strncmp(checksum_header, "SHA1:", 5) == 0
-        || strncmp(checksum_header, "MD5:", 4) == 0;
-}
 
 /**
  * The main function in the reconcile pass.
@@ -297,14 +285,14 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
                     //
                     // In older client versions we always treated these cases as a
                     // non-conflict. This behavior is preserved in case the server
-                    // doesn't provide a suitable content hash.
+                    // doesn't provide a content checksum.
                     //
                     // When it does have one, however, we do create a job, but the job
-                    // will compare hashes and avoid the download if they are equal.
-                    const char *remoteChecksumHeader =
+                    // will compare hashes and avoid the download if possible.
+                    QByteArray remoteChecksumHeader =
                         (ctx->current == REMOTE_REPLICA ? cur->checksumHeader : other->checksumHeader);
-                    if (remoteChecksumHeader) {
-                        is_conflict |= _csync_is_collision_safe_hash(remoteChecksumHeader);
+                    if (!remoteChecksumHeader.isEmpty()) {
+                        is_conflict = true;
                     }
 
                     // SO: If there is no checksum, we can have !is_conflict here

src/csync/csync_util.cpp

@@ -211,3 +211,10 @@ time_t oc_httpdate_parse( const char *date ) {
     result = timegm(&gmt);
     return result;
 }
+
+
+bool csync_is_collision_safe_hash(const QByteArray &checksum_header)
+{
+    return checksum_header.startsWith("SHA1:")
+        || checksum_header.startsWith("MD5:");
+}

src/csync/csync_util.h

@@ -31,4 +31,14 @@ const char OCSYNC_EXPORT *csync_instruction_str(enum csync_instructions_e instr)
 void OCSYNC_EXPORT csync_memstat_check(void);
 
 bool OCSYNC_EXPORT csync_file_locked_or_open( const char *dir, const char *fname);
+
+/* Returns true if we're reasonably certain that hash equality
+ * for the header means content equality.
+ *
+ * Cryptographic safety is not required - this is mainly
+ * intended to rule out checksums like Adler32 that are not intended for
+ * hashing and have high likelihood of collision with particular inputs.
+ */
+bool OCSYNC_EXPORT csync_is_collision_safe_hash(const QByteArray &checksum_header);
+
 #endif /* _CSYNC_UTIL_H */

src/libsync/propagatedownload.cpp

@@ -357,13 +357,16 @@ void PropagateDownloadFile::start()
         }
     }
 
-    // If we have a conflict where size and mtime are identical,
+    // If we have a conflict where size of the file is unchanged,
     // compare the remote checksum to the local one.
     // Maybe it's not a real conflict and no download is necessary!
+    // If the hashes are collision safe and identical, we assume the content is too.
+    // For weak checksums, we only do that if the mtimes are also identical.
     if (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT
         && _item->_size == _item->_previousSize
-        && _item->_modtime == _item->_previousModtime
-        && !_item->_checksumHeader.isEmpty()) {
+        && !_item->_checksumHeader.isEmpty()
+        && (csync_is_collision_safe_hash(_item->_checksumHeader)
+            || _item->_modtime == _item->_previousModtime)) {
         qCDebug(lcPropagateDownload) << _item->_file << "may not need download, computing checksum";
         auto computeChecksum = new ComputeChecksum(this);
         computeChecksum->setChecksumType(parseChecksumHeaderType(_item->_checksumHeader));
@@ -379,8 +382,17 @@ void PropagateDownloadFile::start()
 void PropagateDownloadFile::conflictChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum)
 {
     if (makeChecksumHeader(checksumType, checksum) == _item->_checksumHeader) {
+        // No download necessary, just update fs and journal metadata
         qCDebug(lcPropagateDownload) << _item->_file << "remote and local checksum match";
-        // No download necessary, just update metadata
+
+        // Apply the server mtime locally if necessary, ensuring the journal
+        // and local mtimes end up identical
+        auto fn = propagator()->getFilePath(_item->_file);
+        if (_item->_modtime != _item->_previousModtime) {
+            FileSystem::setModTime(fn, _item->_modtime);
+            emit propagator()->touchedFile(fn);
+        }
+        _item->_modtime = FileSystem::getModTime(fn);
         updateMetadata(/*isConflict=*/false);
         return;
     }

test/testsyncengine.cpp

@@ -311,6 +311,22 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(nGET, 1);
 
+        // Conflict: Same content, different mtime, matching checksums
+        //           -> PropagateDownload, but it skips the download
+        mtime = mtime.addDays(1);
+        fakeFolder.localModifier().setContents("A/a1", 'C');
+        fakeFolder.localModifier().setModTime("A/a1", mtime);
+        fakeFolder.remoteModifier().setContents("A/a1", 'C');
+        fakeFolder.remoteModifier().setModTime("A/a1", mtime.addDays(1));
+        remoteInfo.find("A/a1")->checksums = "SHA1:56900fb1d337cf7237ff766276b9c1e8ce507427";
+        QVERIFY(fakeFolder.syncOnce());
+        // check that mtime in journal and filesystem agree
+        QString a1path = fakeFolder.localPath() + "A/a1";
+        SyncJournalFileRecord a1record;
+        fakeFolder.syncJournal().getFileRecord(QByteArray("A/a1"), &a1record);
+        QCOMPARE(a1record._modtime, (qint64)FileSystem::getModTime(a1path));
+        QCOMPARE(nGET, 1);
+
         // Extra sync reads from db, no difference
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(nGET, 1);