sync: Merge apply updates and resolve conflicts

sync/engine/apply_updates_command.cc

Index: sync/engine/apply_updates_command.cc
diff --git a/sync/engine/apply_updates_command.cc b/sync/engine/apply_updates_command.cc
index bff83f2260a345a79349c1d7b87cbe2c1ed4a9f5..130a5b70c69458b0ab6b95af188d67c384541d4f 100644
--- a/sync/engine/apply_updates_command.cc
+++ b/sync/engine/apply_updates_command.cc
@@ -5,6 +5,7 @@
 #include "sync/engine/apply_updates_command.h"
 
 #include "base/location.h"
+#include "sync/engine/conflict_resolver.h"
 #include "sync/engine/update_applicator.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/directory.h"
@@ -41,6 +42,7 @@ std::set<ModelSafeGroup> ApplyUpdatesCommand::GetGroupsToChange(
 
 SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
+  sessions::StatusController* status = session->mutable_status_controller();
   syncable::Directory* dir = session->context()->directory();
   syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
 
@@ -52,7 +54,7 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(
   for (FullModelTypeSet::Iterator it =
            server_types_with_unapplied_updates.First(); it.Good(); it.Inc()) {
     if (GetGroupForModelType(it.Get(), session->routing_info()) ==
-        session->status_controller().group_restriction()) {
+        status->group_restriction()) {
       server_type_restriction.Put(it.Get());
     }
   }
@@ -65,19 +67,69 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(
   dir->GetUnappliedUpdateMetaHandles(
       &trans, server_type_restriction, &handles);
 
+  // First set of update application passes.
   UpdateApplicator applicator(
       dir->GetCryptographer(&trans),
       session->routing_info(),
-      session->status_controller().group_restriction());
-  applicator.AttemptApplications(&trans, handles,
-                                 session->mutable_status_controller());
+      status->group_restriction());
+  applicator.AttemptApplications(&trans, handles);
+  status->increment_num_updates_applied_by(applicator.updates_applied());
+  status->increment_num_hierarchy_conflicts_by(
+      applicator.hierarchy_conflicts());
+  status->increment_num_encryption_conflicts_by(
+      applicator.encryption_conflicts());
+
+  if (applicator.simple_conflict_ids().size() != 0) {
+    // Resolve the simple conflicts we just detected.

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

In the interest of writing short and simple functions, this if-block seems like
a good candidate for a separate ResolveSimpleConflicts function (this one is at
~100 already).

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

It looks big, but there's very little code in here.

By my count, we have:
- 2 lines to instantiate function-like objects
- 4 lines of actual code
- 3 DCHECKs

(I'm counting lines wrapped because they exceed 80 characters as one line.)

The remainder is comments and whitespace.

[tim (not reviewing), 2012/10/24 22:34:24]

Fair enough. Note though based on several readability reviews I've seen, I think
the point the style guide makes is that when a function "looks big" you pay a
readability price.

It also seems like a natural break-up if we're going to name the command
ApplyUpdatesAndResolveConflicts.  

I think it's preferable, but won't hold up this review because of it.

+    ConflictResolver resolver;
+    resolver.ResolveConflicts(&trans,
+                              dir->GetCryptographer(&trans),
+                              applicator.simple_conflict_ids(),
+                              status);
+
+    // Conflict resolution sometimes results in more updates to apply.
+    handles.clear();
+    dir->GetUnappliedUpdateMetaHandles(
+        &trans, server_type_restriction, &handles);
+
+    UpdateApplicator conflict_applicator(
+        dir->GetCryptographer(&trans),
+        session->routing_info(),
+        status->group_restriction());
+    conflict_applicator.AttemptApplications(&trans, handles);
+
+    // We count the number of updates from both applicator passes.
+    status->increment_num_updates_applied_by(
+        conflict_applicator.updates_applied());
+
+    // Encryption conflicts should remain unchanged by the resolution of simple
+    // conflicts.  Those can only be solved by updating our nigori key bag.
+    DCHECK_EQ(conflict_applicator.encryption_conflicts(),
+              applicator.encryption_conflicts());
+
+    // Hierarchy conflicts should also remain unchanged, for reasons that are
+    // more subtle.  Hierarchy conflicts exist when the application of a pending
+    // update from the server would make the local folder hierarchy
+    // inconsistent.  The resolution of simple conflicts could never affect the
+    // hierarchy conflicting item directly, because hierarchy conflicts are not
+    // processed by the conflict resolver.  It could, in theory, modify the
+    // local hierarchy on which hierarchy conflict detection depends.  However,
+    // the conflict resolution algorithm currently in use does not allow this.
+    DCHECK_EQ(conflict_applicator.hierarchy_conflicts(),
+              applicator.hierarchy_conflicts());
+
+    // There should be no simple conflicts remaining.  We know this because the
+    // resolver should have resolved all the conflicts we detected last time
+    // and, by the two previous assertions, that no conflicts have been
+    // downgraded from encryption or hierarchy down to simple.
+    DCHECK(conflict_applicator.simple_conflict_ids().empty());
+  }
 
   // This might be the first time we've fully completed a sync cycle, for
   // some subset of the currently synced datatypes.
-  const sessions::StatusController& status(session->status_controller());
-  if (status.ServerSaysNothingMoreToDownload()) {
+  if (status->ServerSaysNothingMoreToDownload()) {
     for (ModelTypeSet::Iterator it =
-             status.updates_request_types().First(); it.Good(); it.Inc()) {
+             status->updates_request_types().First(); it.Good(); it.Inc()) {
       // Don't set the flag for control types.  We didn't process them here.
       if (IsControlType(it.Get()))
         continue;