sync: Process 'control' data separately

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 35490c3a8a89961772189db3302c3aa4887e0bd3..2ce5ff0a614baf1c5cc8a1a8e013b0262004a69c 100644
--- a/sync/engine/apply_updates_command.cc
+++ b/sync/engine/apply_updates_command.cc
@@ -36,6 +36,10 @@ std::set<ModelSafeGroup> ApplyUpdatesCommand::GetGroupsToChange(
         GetGroupForModelType(it.Get(), session.routing_info()));
   }
 
+  // The INTERNAL types are special.  They have their own update routines,
+  // ApplyUpdatesCommand should not process them.
+  groups_with_unapplied_updates.erase(GROUP_INTERNAL);

[tim (not reviewing), 2012/06/22 16:34:15]

Hmm.  If we apply these types separately and before ApplyUpdates, what are the
cases where we would actually find unapplied updates for internal types here? 
Shouldn't they have already been processed?

Also, we should keep the naming consistent.  This patch uses 'INTERNAL' and
'Metadata' to refer to the same thing (e.g. to be consistent your step should be
ApplyInternalData, or you should make the group 'metadata'.)  Although we
already use 'internal' and 'metas' to mean other things :/ How about calling
these 'ControlData/GROUP_CONTROL', 'AccountSetup' or 'Base' or something. 

[rlarocque, 2012/06/22 16:42:31]

I think you're right.  Maybe we should assert that GROUP_INTERNAL is not in the
list?
 
I didn't like those names either.  I like GROUP_CONTROL, though.  I'll rename
accordingly.  

[rlarocque, 2012/06/22 18:40:34]

Actually, that won't work.  We release the transaction between
ApplyMetadataUpdates and ApplyUpdatesCommand.  A user with good timing could
update the nigori state between the two steps.

+
   return groups_with_unapplied_updates;
 }