sync: Merge apply updates and resolve conflicts

sync/engine/conflict_resolver.cc

Index: sync/engine/conflict_resolver.cc
diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index eec418ad03ddae893dbd43edffa4eec36b44cc68..def2726180ea82ca159d3797a00d2333179fb794 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -35,11 +35,10 @@ 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());
@@ -50,7 +49,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
       !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)) {
@@ -60,7 +59,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     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.
@@ -206,6 +205,10 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
                                 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 "
@@ -225,22 +228,16 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     // 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.
@@ -274,16 +271,14 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
                                 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;
@@ -323,17 +318,11 @@ bool ConflictResolver::ResolveConflicts(
     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