sync: Merge apply updates and resolve conflicts

sync/engine/apply_updates_command.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/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"
 #include "sync/syncable/read_transaction.h"
 #include "sync/syncable/write_transaction.h"
 
 namespace syncer {
 
 using sessions::SyncSession;
 
@@ skipping 16 matching lines @@
            server_types_with_unapplied_updates.First(); it.Good(); it.Inc()) {
     groups_with_unapplied_updates.insert(
         GetGroupForModelType(it.Get(), session.routing_info()));
   }
 
   return groups_with_unapplied_updates;
 }
 
 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);
 
   // Compute server types with unapplied updates that fall under our
   // group restriction.
   const FullModelTypeSet server_types_with_unapplied_updates =
       dir->GetServerTypesWithUnappliedUpdates(&trans);
   FullModelTypeSet server_type_restriction;
   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());
     }
   }
 
   // Don't process control type updates here.  They will be handled elsewhere.
   FullModelTypeSet control_types = ToFullModelTypeSet(ControlTypes());
   server_type_restriction.RemoveAll(control_types);
 
   std::vector<int64> handles;
   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;
 
       // This gets persisted to the directory's backing store.
       dir->set_initial_sync_ended_for_type(it.Get(), true);
     }
   }
 
   return SYNCER_OK;
 }
 
 }  // namespace syncer