sync: Merge apply updates and resolve conflicts

sync/engine/conflict_resolver.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/conflict_resolver.h"
 
 #include <list>
 #include <set>
 #include <string>
 
@@ skipping 17 matching lines @@
 using syncable::Id;
 using syncable::MutableEntry;
 using syncable::WriteTransaction;
 
 ConflictResolver::ConflictResolver() {
 }
 
 ConflictResolver::~ConflictResolver() {
 }
 
-ConflictResolver::ProcessSimpleConflictResult
-ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
-                                        const Id& id,
-                                        const Cryptographer* cryptographer,
-                                        StatusController* status) {
+void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
+                                             const Id& id,
+                                             const Cryptographer* cryptographer,
+                                             StatusController* status) {
   MutableEntry entry(trans, syncable::GET_BY_ID, id);
   // Must be good as the entry won't have been cleaned up.
   CHECK(entry.good());
 
   // This function can only resolve simple conflicts.  Simple conflicts have
   // both IS_UNSYNCED and IS_UNAPPLIED_UDPATE set.
   if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE) ||
       !entry.Get(syncable::IS_UNSYNCED)) {
     // This is very unusual, but it can happen in tests.  We may be able to
     // assert NOTREACHED() here when those tests are updated.
-    return NO_SYNC_PROGRESS;
+    return;
   }
 
   if (entry.Get(syncable::IS_DEL) && entry.Get(syncable::SERVER_IS_DEL)) {
     // we've both deleted it, so lets just drop the need to commit/update this
     // entry.
     entry.Put(syncable::IS_UNSYNCED, false);
     entry.Put(syncable::IS_UNAPPLIED_UPDATE, false);
     // we've made changes, but they won't help syncing progress.
     // METRIC simple conflict resolved by merge.
-    return NO_SYNC_PROGRESS;
+    return;
   }
 
   // This logic determines "client wins" vs. "server wins" strategy picking.
   // By the time we get to this point, we rely on the following to be true:
   // a) We can decrypt both the local and server data (else we'd be in
   //    conflict encryption and not attempting to resolve).
   // b) All unsynced changes have been re-encrypted with the default key (
   //    occurs either in AttemptToUpdateEntry, SetEncryptionPassphrase,
   //    SetDecryptionPassphrase, or RefreshEncryption).
   // c) Base_server_specifics having a valid datatype means that we received
@@ skipping 125 matching lines @@
                                 CONFLICT_RESOLUTION_SIZE);
     } else if (base_server_specifics_match) {
       DVLOG(1) << "Resolving simple conflict, ignoring server encryption "
                << " changes for: " << entry;
       status->increment_num_server_overwrites();
       OverwriteServerChanges(&entry);
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 IGNORE_ENCRYPTION,
                                 CONFLICT_RESOLUTION_SIZE);
     } else if (entry_deleted || !name_matches || !parent_matches) {
+      // NOTE: The update application logic assumes that conflict resolution
+      // will never result in changes to the local hierarchy.  The entry_deleted
+      // and !paremt_matches cases here are critical to maintaining that

[tim (not reviewing), 2012/10/24 20:29:08]

nit - parent_matches

[rlarocque, 2012/10/24 21:21:22]

Good catch.  Fixed.

+      // assumption.
       OverwriteServerChanges(&entry);
       status->increment_num_server_overwrites();
       DVLOG(1) << "Resolving simple conflict, overwriting server changes "
                << "for: " << entry;
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 OVERWRITE_SERVER,
                                 CONFLICT_RESOLUTION_SIZE);
     } else {
       DVLOG(1) << "Resolving simple conflict, ignoring local changes for: "
                << entry;
       IgnoreLocalChanges(&entry);
       status->increment_num_local_overwrites();
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 OVERWRITE_LOCAL,
                                 CONFLICT_RESOLUTION_SIZE);
     }
     // Now that we've resolved the conflict, clear the prev server
     // specifics.
     entry.Put(syncable::BASE_SERVER_SPECIFICS, sync_pb::EntitySpecifics());
-    return SYNC_PROGRESS;
   } else {  // SERVER_IS_DEL is true
-    // If a server deleted folder has local contents it should be a hierarchy
-    // conflict.  Hierarchy conflicts should not be processed by this function.
-    // We could end up here if a change was made since we last tried to detect
-    // conflicts, which was during update application.
     if (entry.Get(syncable::IS_DIR)) {
       Directory::ChildHandles children;
       trans->directory()->GetChildHandlesById(trans,
                                               entry.Get(syncable::ID),
                                               &children);
-      if (0 != children.size()) {
-        DVLOG(1) << "Entry is a server deleted directory with local contents, "
-                 << "should be a hierarchy conflict. (race condition).";
-        return NO_SYNC_PROGRESS;
-      }
+      // If a server deleted folder has local contents it should be a hierarchy
+      // conflict.  Hierarchy conflicts should not be processed by this
+      // function.
+      DCHECK(children.empty());
     }
 
     // The entry is deleted on the server but still exists locally.
     if (!entry.Get(syncable::UNIQUE_CLIENT_TAG).empty()) {
       // If we've got a client-unique tag, we can undelete while retaining
       // our present ID.
       DCHECK_EQ(entry.Get(syncable::SERVER_VERSION), 0) << "For the server to "
           "know to re-create, client-tagged items should revert to version 0 "
           "when server-deleted.";
       OverwriteServerChanges(&entry);
@@ skipping 13 matching lines @@
 
       MutableEntry server_update(trans, syncable::GET_BY_ID, id);
       CHECK(server_update.good());
       CHECK(server_update.Get(syncable::META_HANDLE) !=
             entry.Get(syncable::META_HANDLE))
           << server_update << entry;
       UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                                 UNDELETE,
                                 CONFLICT_RESOLUTION_SIZE);
     }
-    return SYNC_PROGRESS;
   }
 }
 
-bool ConflictResolver::ResolveConflicts(
+void ConflictResolver::ResolveConflicts(
     syncable::WriteTransaction* trans,
     const Cryptographer* cryptographer,
     const std::set<syncable::Id>& simple_conflict_ids,
     sessions::StatusController* status) {
-  bool forward_progress = false;
   // Iterate over simple conflict items.
   set<Id>::const_iterator conflicting_item_it;
   set<Id> processed_items;
   for (conflicting_item_it = simple_conflict_ids.begin();
        conflicting_item_it != simple_conflict_ids.end();
        ++conflicting_item_it) {
     Id id = *conflicting_item_it;
     if (processed_items.count(id) > 0)
       continue;
 
@@ skipping 19 matching lines @@
       CHECK(entry.good());
       Id new_prev_id = entry.Get(syncable::PREV_ID);
       if (new_prev_id == prev_id)
         break;
       prev_id = new_prev_id;
     } while (processed_items.count(prev_id) == 0 &&
              simple_conflict_ids.count(prev_id) > 0);  // Excludes root.
     while (!predecessors.empty()) {
       id = predecessors.back();
       predecessors.pop_back();
-      switch (ProcessSimpleConflict(trans, id, cryptographer, status)) {
-        case NO_SYNC_PROGRESS:
-          break;
-        case SYNC_PROGRESS:
-          forward_progress = true;
-          break;
-      }
+      ProcessSimpleConflict(trans, id, cryptographer, status);
       processed_items.insert(id);
     }
   }
-  return forward_progress;
+  return;
 }
 
 }  // namespace syncer