Refactor following sync commit loop change

sync/engine/commit.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/commit.h"
 
 #include "base/debug/trace_event.h"
 #include "sync/engine/build_commit_command.h"
 #include "sync/engine/get_commit_ids_command.h"
 #include "sync/engine/process_commit_response_command.h"
 #include "sync/engine/syncer_proto_util.h"
 #include "sync/sessions/sync_session.h"
 
 using syncable::SYNCER;
 using syncable::WriteTransaction;
 
 namespace browser_sync {
 
 using sessions::SyncSession;
 using sessions::StatusController;
 
+namespace {
+
+void IterateOverSyncingBits(WriteTransaction* trans,
+                            const sessions::OrderedCommitSet& commit_set,
+                            bool value) {

[tim (not reviewing), 2012/06/06 00:41:46]

nit - consider renaming parameter value_to_set or the function to
IterateOverSyncingBitsAndSetToValue (or something shorter :/)

Comment might be nice too.

[rlarocque, 2012/06/06 01:01:09]

How about SetAllSyncingBitsToValue()?

+  const std::vector<syncable::Id>& commit_ids = commit_set.GetAllCommitIds();
+  for (std::vector<syncable::Id>::const_iterator it = commit_ids.begin();
+       it != commit_ids.end(); ++it) {
+    syncable::MutableEntry entry(trans, syncable::GET_BY_ID, *it);
+    if (entry.good()) {
+      entry.Put(syncable::SYNCING, value);
+    }
+  }
+}
+
+// Sets the SYNCING bits for all items in the OrderedCommitSet.
+void SetSyncingBits(WriteTransaction* trans,
+                    const sessions::OrderedCommitSet& commit_set) {
+  IterateOverSyncingBits(trans, commit_set, true);
+}
+
+// Clears the SYNCING bits for all items in the OrderedCommitSet.
+void ClearSyncingBits(syncable::Directory* dir,

[tim (not reviewing), 2012/06/06 00:41:46]

nit - We don't really need this function and could inline it at
BuildAndProcessCommits.

[rlarocque, 2012/06/06 01:01:09]

I like this function's name better than IterateOverSyncingBits().  Otherwise I
would inline it.

[tim (not reviewing), 2012/06/08 22:30:51]

Good point, although SetAllSyncingBitsToValue(false) seems more on-par naming
wise...

+                      const sessions::OrderedCommitSet& commit_set) {
+  WriteTransaction trans(FROM_HERE, SYNCER, dir);
+  IterateOverSyncingBits(&trans, commit_set, false);
+}
+
 // Helper function that finds sync items that are ready to be committed to the
 // server and serializes them into a commit message protobuf.  It will return
 // false iff there are no entries ready to be committed at this time.
 //
 // The OrderedCommitSet parameter is an output parameter which will contain
 // the set of all items which are to be committed.  The number of items in
 // the set shall not exceed the maximum batch size.  (The default batch size
 // is currently 25, though it can be overwritten by the server.)
 //
 // The ClientToServerMessage parameter is an output parameter which will contain
@@ skipping 16 matching lines @@
   get_commit_ids_command.Execute(session);
 
   DVLOG(1) << "Commit message will contain " << commit_set->Size() << " items.";
   if (commit_set->Empty())
     return false;
 
   // Serialize the message.
   BuildCommitCommand build_commit_command(*commit_set, commit_message);
   build_commit_command.Execute(session);
 
+  SetSyncingBits(session->write_transaction(), *commit_set);
   return true;
 }
 
-SyncerError BuildAndPostCommits(Syncer* syncer,
-                                sessions::SyncSession* session) {
-  StatusController* status_controller = session->mutable_status_controller();
-
-  sessions::OrderedCommitSet commit_set(session->routing_info());
+SyncerError BuildAndPostCommitsImpl(Syncer* syncer,
+                                    sessions::SyncSession* session,
+                                    sessions::OrderedCommitSet* commit_set) {
   ClientToServerMessage commit_message;
-  while (PrepareCommitMessage(session, &commit_set, &commit_message)
-         && !syncer->ExitRequested()) {
+  while (!syncer->ExitRequested() &&
+         PrepareCommitMessage(session, commit_set, &commit_message)) {
     ClientToServerResponse commit_response;
 
     DVLOG(1) << "Sending commit message.";
     TRACE_EVENT_BEGIN0("sync", "PostCommit");
-    status_controller->set_last_post_commit_result(
-        SyncerProtoUtil::PostClientToServerMessage(commit_message,
-                                                   &commit_response,
-                                                   session));
+    const SyncerError post_result = SyncerProtoUtil::PostClientToServerMessage(
+        commit_message, &commit_response, session);
     TRACE_EVENT_END0("sync", "PostCommit");
 
-    // ProcessCommitResponse includes some code that cleans up after a failure
-    // to post a commit message, so we must run it regardless of whether or not
-    // the commit succeeds.
+    if (post_result != SYNCER_OK) {
+      LOG(WARNING) << "Post commit failed";
+      return post_result;
+    }
+
+    if (!commit_response.has_commit()) {
+      LOG(WARNING) << "Commit response has no commit body!";
+      return SERVER_RESPONSE_VALIDATION_FAILED;
+    }
+
+    const size_t num_responses = commit_response.commit().entryresponse_size();
+    if (num_responses != commit_set->Size()) {
+      LOG(ERROR)
+         << "Commit response has wrong number of entries! "
+         << "Expected: " << commit_set->Size() << ", "
+         << "Got: " << num_responses;
+      return SERVER_RESPONSE_VALIDATION_FAILED;
+    }
 
     TRACE_EVENT_BEGIN0("sync", "ProcessCommitResponse");
     ProcessCommitResponseCommand process_response_command(
-        commit_set, commit_message, commit_response);
-    status_controller->set_last_process_commit_response_result(
-            process_response_command.Execute(session));
+        *commit_set, commit_message, commit_response);
+    const SyncerError processing_result =
+        process_response_command.Execute(session);
     TRACE_EVENT_END0("sync", "ProcessCommitResponse");
 
-    // Exit early if either the commit or the response processing failed.
-    if (status_controller->last_post_commit_result() != SYNCER_OK)
-      return status_controller->last_post_commit_result();
-    if (status_controller->last_process_commit_response_result() != SYNCER_OK)
-      return status_controller->last_process_commit_response_result();
+    if (processing_result != SYNCER_OK) {
+      return processing_result;

[tim (not reviewing), 2012/06/06 00:41:46]

Note that elsewhere in this file we're using no-braces-on-single-line-ifs.
Please follow suit (or make things consistent).

[rlarocque, 2012/06/06 01:01:09]

I think 'with braces' style is more common.  I'll update to using that style
everywhere.

+    }
   }
 
   return SYNCER_OK;
 }
 
+}  // namespace
+
+
+SyncerError BuildAndPostCommits(Syncer* syncer,
+                                sessions::SyncSession* session) {
+  sessions::OrderedCommitSet commit_set(session->routing_info());
+  SyncerError result = BuildAndPostCommitsImpl(syncer, session, &commit_set);
+  if (result != SYNCER_OK) {
+    ClearSyncingBits(session->context()->directory(), commit_set);
+  }
+  return result;
+}
+
 }  // namespace browser_sync