sync: Loop committing items without downloading updates (v2)

sync/engine/syncer_unittest.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.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <algorithm>
 #include <limits>
 #include <list>
@@ skipping 373 matching lines @@
     // If this test starts failing, be aware other sort orders could be valid.
     for (size_t i = 0; i < expected_positions.size(); ++i) {
       EXPECT_EQ(1u, expected_positions.count(i));
       EXPECT_TRUE(expected_positions[i] == mock_server_->committed_ids()[i]);
     }
   }
 
   void DoTruncationTest(const vector<int64>& unsynced_handle_view,
                         const vector<syncable::Id>& expected_id_order) {
     for (size_t limit = expected_id_order.size() + 2; limit > 0; --limit) {
-      StatusController* status = session_->mutable_status_controller();
       WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
       ScopedSetSessionWriteTransaction set_trans(session_.get(), &wtrans);
 
       ModelSafeRoutingInfo routes;
       GetModelSafeRoutingInfo(&routes);
-      GetCommitIdsCommand command(limit);
+      sessions::OrderedCommitSet output_set(routes);
+      GetCommitIdsCommand command(limit, &output_set);
       std::set<int64> ready_unsynced_set;
       command.FilterUnreadyEntries(&wtrans, syncable::ModelTypeSet(),
                                    syncable::ModelTypeSet(), false,
                                    unsynced_handle_view, &ready_unsynced_set);
       command.BuildCommitIds(session_->write_transaction(), routes,
                              ready_unsynced_set);
-      syncable::Directory::UnsyncedMetaHandles ready_unsynced_vector(
-        ready_unsynced_set.begin(), ready_unsynced_set.end());
-      status->set_unsynced_handles(ready_unsynced_vector);
-      vector<syncable::Id> output =
-          command.ordered_commit_set_->GetAllCommitIds();
       size_t truncated_size = std::min(limit, expected_id_order.size());
-      ASSERT_EQ(truncated_size, output.size());
+      ASSERT_EQ(truncated_size, output_set.Size());
       for (size_t i = 0; i < truncated_size; ++i) {
-        ASSERT_EQ(expected_id_order[i], output[i])
+        ASSERT_EQ(expected_id_order[i], output_set.GetCommitIdAt(i))
             << "At index " << i << " with batch size limited to " << limit;
       }
       sessions::OrderedCommitSet::Projection proj;
-      proj = command.ordered_commit_set_->GetCommitIdProjection(GROUP_PASSIVE);
+      proj = output_set.GetCommitIdProjection(GROUP_PASSIVE);
       ASSERT_EQ(truncated_size, proj.size());
       for (size_t i = 0; i < truncated_size; ++i) {
         SCOPED_TRACE(::testing::Message("Projection mismatch with i = ") << i);
-        syncable::Id projected =
-            command.ordered_commit_set_->GetCommitIdAt(proj[i]);
+        syncable::Id projected = output_set.GetCommitIdAt(proj[i]);
         ASSERT_EQ(expected_id_order[proj[i]], projected);
         // Since this projection is the identity, the following holds.
         ASSERT_EQ(expected_id_order[i], projected);
       }
     }
   }
 
   const StatusController& status() {
     return session_->status_controller();
   }
@@ skipping 292 matching lines @@
     C.Put(NON_UNIQUE_NAME, kEncryptedString);
     // Unencrypted non_unique_name.
     MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
     ASSERT_TRUE(D.good());
     D.Put(IS_UNSYNCED, true);
     D.Put(SPECIFICS, encrypted_bookmark);
     D.Put(NON_UNIQUE_NAME, "not encrypted");
   }
   SyncShareNudge();
   {
-    // We remove any unready entries from the status controller's unsynced
-    // handles, so this should remain 0 even though the entries didn't commit.
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Nothing should have commited due to bookmarks being encrypted and
     // the cryptographer having pending keys. A would have been resolved
     // as a simple conflict, but still be unsynced until the next sync cycle.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, true, false, 0, 20, 20, ids_, &rtrans);
     VERIFY_ENTRY(2, false, true, false, 0, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(3, false, true, false, 0, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(4, false, true, false, 0, 10, 10, ids_, &rtrans);
 
     // Resolve the pending keys.
     cryptographer(&rtrans)->DecryptPendingKeys(other_params);
   }
   SyncShareNudge();
   {
-    // 2 unsynced handles to reflect the items that committed succesfully.
-    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
     // All properly encrypted and non-conflicting items should commit. "A" was
     // conflicting, but last sync cycle resolved it as simple conflict, so on
     // this sync cycle it committed succesfullly.
     ReadTransaction rtrans(FROM_HERE, directory());
     // Committed successfully.
     VERIFY_ENTRY(1, false, false, false, 0, 21, 21, ids_, &rtrans);
     // Committed successfully.
     VERIFY_ENTRY(2, false, false, false, 0, 11, 11, ids_, &rtrans);
     // Was not properly encrypted.
     VERIFY_ENTRY(3, false, true, false, 0, 10, 10, ids_, &rtrans);
     // Was not properly encrypted.
     VERIFY_ENTRY(4, false, true, false, 0, 10, 10, ids_, &rtrans);
   }
   {
     // Fix the remaining items.
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry C(&wtrans, GET_BY_ID, ids_.FromNumber(3));
     ASSERT_TRUE(C.good());
     C.Put(SPECIFICS, encrypted_bookmark);
     C.Put(NON_UNIQUE_NAME, kEncryptedString);
     MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
     ASSERT_TRUE(D.good());
     D.Put(SPECIFICS, encrypted_bookmark);
     D.Put(NON_UNIQUE_NAME, kEncryptedString);
   }
   SyncShareNudge();
   {
-    // We attempted to commit two items.
-    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
-    EXPECT_TRUE(session_->status_controller().did_commit_items());
+    const StatusController& status_controller = session_->status_controller();
+    // Expect success.
+    EXPECT_EQ(status_controller.last_post_commit_result(), SYNCER_OK);
     // None should be unsynced anymore.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 21, 21, ids_, &rtrans);
     VERIFY_ENTRY(2, false, false, false, 0, 11, 11, ids_, &rtrans);
     VERIFY_ENTRY(3, false, false, false, 0, 11, 11, ids_, &rtrans);
     VERIFY_ENTRY(4, false, false, false, 0, 11, 11, ids_, &rtrans);
   }
 }
 
 TEST_F(SyncerTest, EncryptionAwareConflicts) {
@@ skipping 28 matching lines @@
     cryptographer(&wtrans)->Update(*nigori);
     EXPECT_TRUE(cryptographer(&wtrans)->has_pending_keys());
   }
 
   mock_server_->AddUpdateSpecifics(1, 0, "A", 10, 10, true, 0, bookmark);
   mock_server_->AddUpdateSpecifics(2, 1, "B", 10, 10, false, 2, bookmark);
   mock_server_->AddUpdateSpecifics(3, 1, "C", 10, 10, false, 1, bookmark);
   mock_server_->AddUpdateSpecifics(4, 0, "D", 10, 10, false, 0, pref);
   SyncShareNudge();
   {
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Initial state. Everything is normal.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(2, false, false, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(3, false, false, false, 1, 10, 10, ids_, &rtrans);
     VERIFY_ENTRY(4, false, false, false, 0, 10, 10, ids_, &rtrans);
   }
 
   // Server side encryption will not be applied due to undecryptable data.
   // At this point, BASE_SERVER_SPECIFICS should be filled for all four items.
   mock_server_->AddUpdateSpecifics(1, 0, kEncryptedString, 20, 20, true, 0,
                                    encrypted_bookmark);
   mock_server_->AddUpdateSpecifics(2, 1, kEncryptedString, 20, 20, false, 2,
                                    encrypted_bookmark);
   mock_server_->AddUpdateSpecifics(3, 1, kEncryptedString, 20, 20, false, 1,
                                    encrypted_bookmark);
   mock_server_->AddUpdateSpecifics(4, 0, kEncryptedString, 20, 20, false, 0,
                                    encrypted_pref);
   SyncShareNudge();
   {
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // All should be unapplied due to being undecryptable and have a valid
     // BASE_SERVER_SPECIFICS.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, true, false, true, 0, 10, 20, ids_, &rtrans);
     VERIFY_ENTRY(2, true, false, true, 1, 10, 20, ids_, &rtrans);
     VERIFY_ENTRY(3, true, false, true, 1, 10, 20, ids_, &rtrans);
     VERIFY_ENTRY(4, true, false, true, 0, 10, 20, ids_, &rtrans);
   }
 
   // Server side change that don't modify anything should not affect
   // BASE_SERVER_SPECIFICS (such as name changes and mtime changes).
   mock_server_->AddUpdateSpecifics(1, 0, kEncryptedString, 30, 30, true, 0,
                                    encrypted_bookmark);
   mock_server_->AddUpdateSpecifics(2, 1, kEncryptedString, 30, 30, false, 2,
                                    encrypted_bookmark);
   // Item 3 doesn't change.
   mock_server_->AddUpdateSpecifics(4, 0, kEncryptedString, 30, 30, false, 0,
                                    encrypted_pref);
   SyncShareNudge();
   {
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Items 1, 2, and 4 should have newer server versions, 3 remains the same.
     // All should remain unapplied due to be undecryptable.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, true, false, true, 0, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(2, true, false, true, 1, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(3, true, false, true, 1, 10, 20, ids_, &rtrans);
     VERIFY_ENTRY(4, true, false, true, 0, 10, 30, ids_, &rtrans);
   }
 
   // Positional changes, parent changes, and specifics changes should reset
   // BASE_SERVER_SPECIFICS.
   // Became unencrypted.
   mock_server_->AddUpdateSpecifics(1, 0, "A", 40, 40, true, 0, bookmark);
   // Reordered to after item 2.
   mock_server_->AddUpdateSpecifics(3, 1, kEncryptedString, 30, 30, false, 3,
                                    encrypted_bookmark);
   SyncShareNudge();
   {
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Items 2 and 4 should be the only ones with BASE_SERVER_SPECIFICS set.
     // Items 1 is now unencrypted, so should have applied normally.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, false, false, 0, 40, 40, ids_, &rtrans);
     VERIFY_ENTRY(2, true, false, true, 1, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(3, true, false, false, 1, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(4, true, false, true, 0, 10, 30, ids_, &rtrans);
   }
 
   // Make local changes, which should remain unsynced for items 2, 3, 4.
@@ skipping 15 matching lines @@
     C.Put(NON_UNIQUE_NAME, kEncryptedString);
     C.Put(IS_UNSYNCED, true);
     MutableEntry D(&wtrans, GET_BY_ID, ids_.FromNumber(4));
     ASSERT_TRUE(D.good());
     D.Put(SPECIFICS, modified_pref);
     D.Put(NON_UNIQUE_NAME, kEncryptedString);
     D.Put(IS_UNSYNCED, true);
   }
   SyncShareNudge();
   {
-    EXPECT_EQ(0U, session_->status_controller().unsynced_handles().size());
     // Item 1 remains unsynced due to there being pending keys.
     // Items 2, 3, 4 should remain unsynced since they were not up to date.
     ReadTransaction rtrans(FROM_HERE, directory());
     VERIFY_ENTRY(1, false, true, false, 0, 40, 40, ids_, &rtrans);
     VERIFY_ENTRY(2, true, true, true, 1, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(3, true, true, false, 1, 10, 30, ids_, &rtrans);
     VERIFY_ENTRY(4, true, true, true, 0, 10, 30, ids_, &rtrans);
   }
 
   {
     ReadTransaction rtrans(FROM_HERE, directory());
     // Resolve the pending keys.
     cryptographer(&rtrans)->DecryptPendingKeys(key_params);
   }
   // First cycle resolves conflicts, second cycle commits changes.
   SyncShareNudge();
-  EXPECT_EQ(2, session_->status_controller().syncer_status().
-      num_server_overwrites);
-  EXPECT_EQ(1, session_->status_controller().syncer_status().
-      num_local_overwrites);
-  // We attempted to commit item 1.
-  EXPECT_EQ(1U, session_->status_controller().unsynced_handles().size());
-  EXPECT_TRUE(session_->status_controller().did_commit_items());
+  EXPECT_EQ(2, status().syncer_status().num_server_overwrites);
+  EXPECT_EQ(1, status().syncer_status().num_local_overwrites);
+  // We successfully commited item(s).
+  EXPECT_EQ(status().last_post_commit_result(), SYNCER_OK);
   SyncShareNudge();
-  {
-    // Everything should be resolved now. The local changes should have
-    // overwritten the server changes for 2 and 4, while the server changes
-    // overwrote the local for entry 3.
-    // We attempted to commit two handles.
-    EXPECT_EQ(0, session_->status_controller().syncer_status().
-        num_server_overwrites);
-    EXPECT_EQ(0, session_->status_controller().syncer_status().
-        num_local_overwrites);
-    EXPECT_EQ(2U, session_->status_controller().unsynced_handles().size());
-    EXPECT_TRUE(session_->status_controller().did_commit_items());
-    ReadTransaction rtrans(FROM_HERE, directory());
-    VERIFY_ENTRY(1, false, false, false, 0, 41, 41, ids_, &rtrans);
-    VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
-    VERIFY_ENTRY(3, false, false, false, 1, 30, 30, ids_, &rtrans);
-    VERIFY_ENTRY(4, false, false, false, 0, 31, 31, ids_, &rtrans);
-  }
+
+  // Everything should be resolved now. The local changes should have
+  // overwritten the server changes for 2 and 4, while the server changes
+  // overwrote the local for entry 3.
+  EXPECT_EQ(0, status().syncer_status().num_server_overwrites);
+  EXPECT_EQ(0, status().syncer_status().num_local_overwrites);
+  EXPECT_EQ(status().last_post_commit_result(), SYNCER_OK);
+  ReadTransaction rtrans(FROM_HERE, directory());
+  VERIFY_ENTRY(1, false, false, false, 0, 41, 41, ids_, &rtrans);
+  VERIFY_ENTRY(2, false, false, false, 1, 31, 31, ids_, &rtrans);
+  VERIFY_ENTRY(3, false, false, false, 1, 30, 30, ids_, &rtrans);
+  VERIFY_ENTRY(4, false, false, false, 0, 31, 31, ids_, &rtrans);
 }
 
 #undef VERIFY_ENTRY
 
 // Receive an old nigori with old encryption keys and encrypted types. We should
 // not revert our default key or encrypted types.
 TEST_F(SyncerTest, ReceiveOldNigori) {
   KeyParams old_key = {"localhost", "dummy", "old"};
   KeyParams current_key = {"localhost", "dummy", "cur"};
 
@@ skipping 208 matching lines @@
     parent.Put(syncable::BASE_VERSION, 1);
     parent.Put(syncable::ID, parent_id_);
     MutableEntry child(&wtrans, syncable::CREATE, parent_id_, "Pete");
     ASSERT_TRUE(child.good());
     child.Put(syncable::ID, child_id_);
     child.Put(syncable::BASE_VERSION, 1);
     WriteTestDataToEntry(&wtrans, &child);
   }
 
   SyncShareNudge();
-  EXPECT_EQ(2u, status().unsynced_handles().size());
   ASSERT_EQ(2u, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(child_id_ == mock_server_->committed_ids()[1]);
   {
     ReadTransaction rt(FROM_HERE, directory());
     Entry entry(&rt, syncable::GET_BY_ID, child_id_);
     ASSERT_TRUE(entry.good());
     VerifyTestDataInEntry(&rt, &entry);
   }
@@ skipping 23 matching lines @@
     parent2.Put(syncable::IS_DIR, true);
     parent2.Put(syncable::SPECIFICS, DefaultPreferencesSpecifics());
     parent2.Put(syncable::BASE_VERSION, 1);
     parent2.Put(syncable::ID, pref_node_id);
   }
 
   directory()->PurgeEntriesWithTypeIn(
       syncable::ModelTypeSet(syncable::PREFERENCES));
 
   SyncShareNudge();
-  EXPECT_EQ(2U, status().unsynced_handles().size());
   ASSERT_EQ(2U, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(child_id_ == mock_server_->committed_ids()[1]);
   {
     ReadTransaction rt(FROM_HERE, directory());
     Entry entry(&rt, syncable::GET_BY_ID, child_id_);
     ASSERT_TRUE(entry.good());
     VerifyTestDataInEntry(&rt, &entry);
   }
@@ skipping 218 matching lines @@
       grandchild.Put(syncable::IS_UNSYNCED, true);
       grandchild.Put(syncable::IS_DEL, true);
       grandchild.Put(syncable::IS_DIR, false);
       grandchild.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
       grandchild.Put(syncable::BASE_VERSION, 1);
       grandchild.Put(syncable::MTIME, now_minus_2h);
     }
   }
 
   SyncShareNudge();
-  EXPECT_EQ(6u, session_->status_controller().unsynced_handles().size());
   ASSERT_EQ(6u, mock_server_->committed_ids().size());
   // This test will NOT unroll deletes because SERVER_PARENT_ID is not set.
   // It will treat these like moves.
   vector<syncable::Id> commit_ids(mock_server_->committed_ids());
   EXPECT_TRUE(ids_.FromNumber(100) == commit_ids[0]);
   EXPECT_TRUE(ids_.FromNumber(101) == commit_ids[1]);
   EXPECT_TRUE(ids_.FromNumber(102) == commit_ids[2]);
   // We don't guarantee the delete orders in this test, only that they occur
   // at the end.
   std::sort(commit_ids.begin() + 3, commit_ids.end());
@@ skipping 47 matching lines @@
     MutableEntry child(&wtrans, syncable::CREATE, child_id_, "B");
     ASSERT_TRUE(child.good());
     child.Put(syncable::IS_UNSYNCED, true);
     child.Put(syncable::IS_DIR, true);
     child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     child.Put(syncable::ID, ids_.FromNumber(105));
     child.Put(syncable::BASE_VERSION, 1);
   }
 
   SyncShareNudge();
-  EXPECT_EQ(6u, session_->status_controller().unsynced_handles().size());
   ASSERT_EQ(6u, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(child_id_ == mock_server_->committed_ids()[1]);
   EXPECT_TRUE(ids_.FromNumber(102) == mock_server_->committed_ids()[2]);
   EXPECT_TRUE(ids_.FromNumber(-103) == mock_server_->committed_ids()[3]);
   EXPECT_TRUE(ids_.FromNumber(-104) == mock_server_->committed_ids()[4]);
   EXPECT_TRUE(ids_.FromNumber(105) == mock_server_->committed_ids()[5]);
 }
 
@@ skipping 18 matching lines @@
     child2.Put(syncable::IS_UNSYNCED, true);
     child2.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     child2.Put(syncable::ID, child2_id);
 
     parent.Put(syncable::BASE_VERSION, 1);
     child1.Put(syncable::BASE_VERSION, 1);
     child2.Put(syncable::BASE_VERSION, 1);
   }
 
   SyncShareNudge();
-  EXPECT_EQ(3u, session_->status_controller().unsynced_handles().size());
   ASSERT_EQ(3u, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(child_id_ == mock_server_->committed_ids()[1]);
   EXPECT_TRUE(child2_id == mock_server_->committed_ids()[2]);
 }
 
 TEST_F(SyncerTest, TestCommitListOrderingAndNewParent) {
   string parent1_name = "1";
   string parent2_name = "A";
@@ skipping 25 matching lines @@
     MutableEntry child(&wtrans, syncable::CREATE, parent2_id, child_name);
     ASSERT_TRUE(child.good());
     child.Put(syncable::IS_UNSYNCED, true);
     child.Put(syncable::IS_DIR, true);
     child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     child.Put(syncable::ID, child_id);
     child.Put(syncable::BASE_VERSION, 1);
   }
 
   SyncShareNudge();
-  EXPECT_EQ(3u, session_->status_controller().unsynced_handles().size());
   ASSERT_EQ(3u, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(parent2_id == mock_server_->committed_ids()[1]);
   EXPECT_TRUE(child_id == mock_server_->committed_ids()[2]);
   {
     ReadTransaction rtrans(FROM_HERE, directory());
     // Check that things committed correctly.
     Entry entry_1(&rtrans, syncable::GET_BY_ID, parent_id_);
     EXPECT_EQ(entry_1.Get(NON_UNIQUE_NAME), parent1_name);
@@ skipping 50 matching lines @@
     MutableEntry child(&wtrans, syncable::CREATE, parent2_local_id, child_name);
     ASSERT_TRUE(child.good());
     child.Put(syncable::IS_UNSYNCED, true);
     child.Put(syncable::IS_DIR, true);
     child.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
     child.Put(syncable::ID, child_local_id);
     meta_handle_b = child.Get(syncable::META_HANDLE);
   }
 
   SyncShareNudge();
-  EXPECT_EQ(3u, session_->status_controller().unsynced_handles().size());
   ASSERT_EQ(3u, mock_server_->committed_ids().size());
   // If this test starts failing, be aware other sort orders could be valid.
   EXPECT_TRUE(parent_id_ == mock_server_->committed_ids()[0]);
   EXPECT_TRUE(parent2_local_id == mock_server_->committed_ids()[1]);
   EXPECT_TRUE(child_local_id == mock_server_->committed_ids()[2]);
   {
     ReadTransaction rtrans(FROM_HERE, directory());
 
     Entry parent(&rtrans, syncable::GET_BY_ID,
                  GetOnlyEntryWithName(&rtrans, rtrans.root_id(), parent_name));
@@ skipping 606 matching lines @@
                        "bob");
     ASSERT_TRUE(entry.good());
     entry.Put(syncable::IS_DIR, true);
     entry.Put(syncable::IS_UNSYNCED, true);
     entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
   }
 
   mock_server_->SetMidCommitCallback(
       base::Bind(&EntryCreatedInNewFolderTest::CreateFolderInBob,
                  base::Unretained(this)));
-  syncer_->SyncShare(session_.get(), BUILD_COMMIT_REQUEST, SYNCER_END);
-  EXPECT_EQ(1u, mock_server_->committed_ids().size());
+  syncer_->SyncShare(session_.get(), COMMIT, SYNCER_END);
+  // We loop until no unsynced handles remain, so we will commit both ids.
+  EXPECT_EQ(2u, mock_server_->committed_ids().size());
   {
     ReadTransaction trans(FROM_HERE, directory());
     Entry parent_entry(&trans, syncable::GET_BY_ID,
         GetOnlyEntryWithName(&trans, TestIdFactory::root(), "bob"));
     ASSERT_TRUE(parent_entry.good());
 
     Id child_id =
         GetOnlyEntryWithName(&trans, parent_entry.Get(ID), "bob");
     Entry child(&trans, syncable::GET_BY_ID, child_id);
     ASSERT_TRUE(child.good());
     EXPECT_EQ(parent_entry.Get(ID), child.Get(PARENT_ID));
-}
+  }
 }
 
 TEST_F(SyncerTest, NegativeIDInUpdate) {
   mock_server_->AddUpdateBookmark(-10, 0, "bad", 40, 40);
   SyncShareNudge();
   // The negative id would make us CHECK!
 }
 
 TEST_F(SyncerTest, UnappliedUpdateOnCreatedItemItemDoesNotCrash) {
   int64 metahandle_fred;
@@ skipping 325 matching lines @@
     EXPECT_TRUE("bob" == id2.Get(NON_UNIQUE_NAME));
     EXPECT_TRUE(root_id_ == id2.Get(PARENT_ID));
     Entry id3(&trans, GET_BY_ID, ids_.FromNumber(4096));
     ASSERT_TRUE(id3.good());
     EXPECT_TRUE("bob" == id3.Get(NON_UNIQUE_NAME));
     EXPECT_TRUE(root_id_ == id3.Get(PARENT_ID));
   }
   saw_syncer_event_ = false;
 }
 
-TEST_F(SyncerTest, CommitManyItemsInOneGo) {
-  uint32 max_batches = 3;
-  uint32 items_to_commit = kDefaultMaxCommitBatchSize * max_batches;
+// Committing more than kDefaultMaxCommitBatchSize items requires that
+// we post more than one commit command to the server.  This test makes
+// sure that scenario works as expected.
+TEST_F(SyncerTest, CommitManyItemsInOneGo_Success) {
+  uint32 num_batches = 3;
+  uint32 items_to_commit = kDefaultMaxCommitBatchSize * num_batches;
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     for (uint32 i = 0; i < items_to_commit; i++) {
       string nameutf8 = base::StringPrintf("%d", i);
       string name(nameutf8.begin(), nameutf8.end());
       MutableEntry e(&trans, CREATE, trans.root_id(), name);
       e.Put(IS_UNSYNCED, true);
       e.Put(IS_DIR, true);
       e.Put(SPECIFICS, DefaultBookmarkSpecifics());
     }
   }
-  uint32 num_loops = 0;
-  while (SyncShareNudge()) {
-    num_loops++;
-    ASSERT_LT(num_loops, max_batches * 2);
+  ASSERT_EQ(items_to_commit, directory()->unsynced_entity_count());
+
+  EXPECT_FALSE(SyncShareNudge());
+  EXPECT_EQ(num_batches, mock_server_->commit_messages().size());
+  EXPECT_EQ(0, directory()->unsynced_entity_count());
+}
+
+// Test that a single failure to contact the server will cause us to exit the
+// commit loop immediately.
+TEST_F(SyncerTest, CommitManyItemsInOneGo_PostBufferFail) {
+  uint32 num_batches = 3;
+  uint32 items_to_commit = kDefaultMaxCommitBatchSize * num_batches;
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    for (uint32 i = 0; i < items_to_commit; i++) {
+      string nameutf8 = base::StringPrintf("%d", i);
+      string name(nameutf8.begin(), nameutf8.end());
+      MutableEntry e(&trans, CREATE, trans.root_id(), name);
+      e.Put(IS_UNSYNCED, true);
+      e.Put(IS_DIR, true);
+      e.Put(SPECIFICS, DefaultBookmarkSpecifics());
+    }
   }
-  EXPECT_GE(mock_server_->commit_messages().size(), max_batches);
+  ASSERT_EQ(items_to_commit, directory()->unsynced_entity_count());
+
+  // The second commit should fail.  It will be preceded by one successful
+  // GetUpdate and one succesful commit.
+  mock_server_->FailNthPostBufferToPathCall(3);
+  SyncShareNudge();
+
+  EXPECT_EQ(1U, mock_server_->commit_messages().size());
+  EXPECT_FALSE(session_->Succeeded());
+  EXPECT_EQ(SYNC_SERVER_ERROR,
+            session_->status_controller().error().last_post_commit_result);
+  EXPECT_EQ(items_to_commit - kDefaultMaxCommitBatchSize,
+            directory()->unsynced_entity_count());
+}
+
+// Test that a single conflict response from the server will cause us to exit
+// the commit loop immediately.
+TEST_F(SyncerTest, CommitManyItemsInOneGo_CommitConflict) {
+  uint32 num_batches = 2;
+  uint32 items_to_commit = kDefaultMaxCommitBatchSize * num_batches;
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    for (uint32 i = 0; i < items_to_commit; i++) {
+      string nameutf8 = base::StringPrintf("%d", i);
+      string name(nameutf8.begin(), nameutf8.end());
+      MutableEntry e(&trans, CREATE, trans.root_id(), name);
+      e.Put(IS_UNSYNCED, true);
+      e.Put(IS_DIR, true);
+      e.Put(SPECIFICS, DefaultBookmarkSpecifics());
+    }
+  }
+  ASSERT_EQ(items_to_commit, directory()->unsynced_entity_count());
+
+  // Return a CONFLICT response for the first item.
+  mock_server_->set_conflict_n_commits(1);
+  SyncShareNudge();
+
+  // We should stop looping at the first sign of trouble.
+  EXPECT_EQ(1U, mock_server_->commit_messages().size());
+  EXPECT_FALSE(session_->Succeeded());
+  EXPECT_EQ(items_to_commit - (kDefaultMaxCommitBatchSize - 1),
+            directory()->unsynced_entity_count());
 }
 
 TEST_F(SyncerTest, HugeConflict) {
   int item_count = 300;  // We should be able to do 300 or 3000 w/o issue.
 
   syncable::Id parent_id = ids_.NewServerId();
   syncable::Id last_id = parent_id;
   vector<syncable::Id> tree_ids;
 
   // Create a lot of updates for which the parent does not exist yet.
@@ skipping 1535 matching lines @@
   EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   ExpectSyncedAndCreated();
 
   // Delete, begin committing the delete, then undelete while committing.
   Delete();
   ExpectUnsyncedDeletion();
   mock_server_->SetMidCommitCallback(
       base::Bind(&SyncerUndeletionTest::Undelete, base::Unretained(this)));
   SyncShareNudge();
 
-  // The item ought to exist as an unsynced undeletion (meaning,
-  // we think that the next commit ought to be a recreation commit).
+  // We will continue to commit until all nodes are synced, so we expect
+  // that both the delete and following undelete were committed.  We haven't
+  // downloaded any updates, though, so the SERVER fields will be the same
+  // as they were at the start of the cycle.
   EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
-  ExpectUnsyncedUndeletion();
+
+  // Server fields lag behind.
+  EXPECT_FALSE(Get(metahandle_, SERVER_IS_DEL));
+
+  // We have committed the second (undelete) update.
+  EXPECT_FALSE(Get(metahandle_, IS_DEL));
+  EXPECT_FALSE(Get(metahandle_, IS_UNSYNCED));
+  EXPECT_FALSE(Get(metahandle_, IS_UNAPPLIED_UPDATE));
 
   // Now, encounter a GetUpdates corresponding to the deletion from
   // the server.  The undeletion should prevail again and be committed.
   // None of this should trigger any conflict detection -- it is perfectly
   // normal to recieve updates from our own commits.
   mock_server_->SetMidCommitCallback(base::Closure());
-  mock_server_->AddUpdateTombstone(Get(metahandle_, ID));
+  mock_server_->AddUpdateFromLastCommit();
   SyncShareNudge();
   EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems());
   EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests());
   ExpectSyncedAndCreated();
 }
 
 TEST_F(SyncerUndeletionTest, UndeleteBeforeCommit) {
   Create();
   ExpectUnsyncedCreation();
   SyncShareNudge();
@@ skipping 473 matching lines @@
 
 TEST_F(SyncerPositionTiebreakingTest, MidLowHigh) {
   Add(mid_id_);
   Add(low_id_);
   Add(high_id_);
   SyncShareNudge();
   ExpectLocalOrderIsByServerId();
 }
 
 }  // namespace browser_sync