Refactor following sync commit loop change

sync/engine/commit.cc

Index: sync/engine/commit.cc
diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index ae6b8ae6d8d6f99213069b22e77d2805ef1eb7b3..6845446abd4b7eb71e0604c459f104ea03acdbaa 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -19,6 +19,34 @@ namespace browser_sync {
 using sessions::SyncSession;
 using sessions::StatusController;
 
+namespace {
+
+void IterateOverSyncingBits(WriteTransaction* trans,
+                            const sessions::OrderedCommitSet& commit_set,
+                            bool value) {
+  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,
+                      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.
@@ -55,43 +83,57 @@ bool PrepareCommitMessage(sessions::SyncSession* session,
   BuildCommitCommand build_commit_command(*commit_set, commit_message);
   build_commit_command.Execute(session);
 
+  SetSyncingBits(session->write_transaction(), *commit_set);
   return true;
 }
 
+}  // namespace
+
 SyncerError BuildAndPostCommits(Syncer* syncer,
                                 sessions::SyncSession* session) {
-  StatusController* status_controller = session->mutable_status_controller();
-
   sessions::OrderedCommitSet commit_set(session->routing_info());
   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));
+    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";
+      ClearSyncingBits(session->context()->directory(), commit_set);

[tim (not reviewing), 2012/06/05 19:18:25]

Now that you've grouped everything here, instead of having to rely on calling
ClearSyncingBits at every current and future failure exit point, you could
create a ScopedSyncingBitClearer helper(directory, commit_set); 

that would call ClearSyncingBits on destruction (or just inline that function
into the dtor), unless we made it through successfully and made one single call
to helper.CoastIsClear() (or whatever) at the end of this block. Kind of like a
special version of base::AutoReset.

[rlarocque, 2012/06/05 20:51:14]

I considered something like that.  I was actually hoping to create an object
that would set the bits on construction and clear the bits on destruction.  The
problem was that there was no scope where I could put it.  The setting must be
done in the same transaction as BuildCommitCommand, which isn't very
conveniently located.  

Also, there's no need to clear the bits after a successful process commit
response, which is the normal case.  It seems wrong to open a transaction to
redundantly clear those bits.  I guess this could be avoided by putting
'release()' function on that scoped object, but then it starts to feel like it's
more complicated than its worth.  

Because of those complications, I decided that duplicating the code was the
least bad option.

[tim (not reviewing), 2012/06/05 21:16:46]

I see. I was imagining a class with a single "CoastIsClear" method that would
get called in only one place, but alternatively, we could factor the
ClearSyncingBits call out to a function that Clear()'d if the return value was
not SYNCER_OK. 

Either seem slightly more robust than maintaining calls to Clear when adding a
new early return, for example.

+      return post_result;
+    }
+
+    if (!commit_response.has_commit()) {
+      LOG(WARNING) << "Commit response has no commit body!";
+      ClearSyncingBits(session->context()->directory(), commit_set);
+      return SERVER_RESPONSE_VALIDATION_FAILED;
+    }
+
+    size_t num_responses = commit_response.commit().entryresponse_size();

[tim (not reviewing), 2012/06/05 19:18:25]

nit - const

[rlarocque, 2012/06/05 20:51:14]

Fixed here and in a few other places.

+    if (num_responses != commit_set.Size()) {
+      LOG(ERROR)
+         << "Commit response has wrong number of entries! "
+         << "Expected: " << commit_set.Size() << ", "
+         << "Got: " << num_responses;
+      ClearSyncingBits(session->context()->directory(), commit_set);
+      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));
+    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;
+    }
   }
 
   return SYNCER_OK;