sync: Remove IDs from OrderedCommitSet

Remove the IDs from OrderedCommitSet in order to add a new member,
AddCommitItems().

Removing the IDs from this class would have been a good idea on its own.
There hasn't been any need to track both IDs and metahandles for quite
some time, and storing redundant information always carries the risk
that the two copies will disagree with each other. It is particularly
risky in this case because the entries' IDs will be changed when a
commit response is successful.

The AddCommitItems() function would have needed to have an inconvenient
signature if it had to pass in both the handles and IDs of all the
entries being added to the set.  It's much easier to implement now that
we only need to pass in the entries' metahandles.

The new member function is not used in this commit.  It will be used in
a future commit that allows us to run GetCommitIdsCommand on a single
model type at a time.

BUG=278484

Review URL: https://chromiumcodereview.appspot.com/23694004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221154 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 38 insertions, 45 deletions

diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc
index 53c5221..70333c6 100644
--- a/sync/engine/build_commit_command.cc
+++ b/sync/engine/build_commit_command.cc
@@ -119,10 +119,10 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
   AddClientConfigParamsToMessage(session, commit_message);
 
   for (size_t i = 0; i < batch_commit_set_.Size(); i++) {
-    Id id = batch_commit_set_.GetCommitIdAt(i);
+    int64 handle = batch_commit_set_.GetCommitHandleAt(i);
     sync_pb::SyncEntity* sync_entry = commit_message->add_entries();
 
-    Entry meta_entry(trans_, syncable::GET_BY_ID, id);
+    Entry meta_entry(trans_, syncable::GET_BY_HANDLE, handle);
     CHECK(meta_entry.good());
 
     DCHECK_NE(0UL,
diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index ee69812..f7f1e74 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -26,10 +26,10 @@ namespace {
 void SetAllSyncingBitsToValue(WriteTransaction* trans,
                               const sessions::OrderedCommitSet& commit_set,
                               bool value_to_set) {
-  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);
+  const std::vector<int64>& commit_handles = commit_set.GetAllCommitHandles();
+  for (std::vector<int64>::const_iterator it = commit_handles.begin();
+       it != commit_handles.end(); ++it) {
+    syncable::MutableEntry entry(trans, syncable::GET_BY_HANDLE, *it);
     if (entry.good()) {
       entry.Put(syncable::SYNCING, value_to_set);
     }
diff --git a/sync/engine/get_commit_ids_command.cc b/sync/engine/get_commit_ids_command.cc
index b2f0cbf..931886d 100644
--- a/sync/engine/get_commit_ids_command.cc
+++ b/sync/engine/get_commit_ids_command.cc
@@ -74,12 +74,6 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
                  session->context()->routing_info(),
                  ready_unsynced_set);
 
-  const vector<syncable::Id>& verified_commit_ids =
-      commit_set_->GetAllCommitIds();
-
-  for (size_t i = 0; i < verified_commit_ids.size(); i++)
-    DVLOG(1) << "Debug commit batch result:" << verified_commit_ids[i];
-
   return SYNCER_OK;
 }
 
@@ -225,8 +219,7 @@ void GetCommitIdsCommand::TryAddItem(const std::set<int64>& ready_unsynced_set,
   DCHECK(item.Get(syncable::IS_UNSYNCED));
   int64 item_handle = item.Get(syncable::META_HANDLE);
   if (ready_unsynced_set.count(item_handle) != 0) {
-    result->AddCommitItem(item_handle, item.Get(syncable::ID),
-                          item.GetModelType());
+    result->AddCommitItem(item_handle, item.GetModelType());
   }
 }
 
@@ -366,9 +359,7 @@ void GetCommitIdsCommand::AddDeletes(
           DVLOG(1) << "Inserting moved and deleted entry, will be missed by "
                    << "delete roll." << entry.Get(syncable::ID);
 
-          commit_set_->AddCommitItem(metahandle,
-              entry.Get(syncable::ID),
-              entry.GetModelType());
+          commit_set_->AddCommitItem(metahandle, entry.GetModelType());
         }
 
         // Skip this entry since it's a child of a parent that will be
@@ -404,8 +395,7 @@ void GetCommitIdsCommand::AddDeletes(
     if (entry.Get(syncable::IS_DEL)) {
       syncable::Id parent_id = entry.Get(syncable::PARENT_ID);
       if (legal_delete_parents.count(parent_id)) {
-        commit_set_->AddCommitItem(metahandle, entry.Get(syncable::ID),
-            entry.GetModelType());
+        commit_set_->AddCommitItem(metahandle, entry.GetModelType());
       }
     }
   }
diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc
index 4a921cf..5411773 100644
--- a/sync/engine/process_commit_response_command.cc
+++ b/sync/engine/process_commit_response_command.cc
@@ -38,6 +38,7 @@ using syncable::MutableEntry;
 using syncable::Entry;
 using syncable::BASE_VERSION;
 using syncable::GET_BY_ID;
+using syncable::GET_BY_HANDLE;
 using syncable::ID;
 using syncable::IS_DEL;
 using syncable::IS_DIR;
@@ -100,7 +101,7 @@ SyncerError ProcessCommitResponseCommand::ModelChangingExecuteImpl(
           &trans,
           cr.entryresponse(proj[i]),
           commit_message.entries(proj[i]),
-          commit_set_.GetCommitIdAt(proj[i]),
+          commit_set_.GetCommitHandleAt(proj[i]),
           &deleted_folders);
       switch (response_type) {
         case CommitResponse::INVALID_MESSAGE:
@@ -168,10 +169,9 @@ ProcessCommitResponseCommand::ProcessSingleCommitResponse(
     syncable::WriteTransaction* trans,
     const sync_pb::CommitResponse_EntryResponse& server_entry,
     const sync_pb::SyncEntity& commit_request_entry,
-    const syncable::Id& pre_commit_id,
+    const int64 metahandle,
     set<syncable::Id>* deleted_folders) {
-
-  MutableEntry local_entry(trans, GET_BY_ID, pre_commit_id);
+  MutableEntry local_entry(trans, GET_BY_HANDLE, metahandle);
   CHECK(local_entry.good());
   bool syncing_was_set = local_entry.Get(SYNCING);
   local_entry.Put(SYNCING, false);
@@ -216,11 +216,13 @@ ProcessCommitResponseCommand::ProcessSingleCommitResponse(
   // it as an error response and retry later.
   const syncable::Id& server_entry_id =
       SyncableIdFromProto(server_entry.id_string());
-  if (pre_commit_id != server_entry_id) {
+  if (local_entry.Get(ID) != server_entry_id) {
     Entry e(trans, GET_BY_ID, server_entry_id);
     if (e.good()) {
-      LOG(ERROR) << "Got duplicate id when commiting id: " << pre_commit_id <<
-                 ". Treating as an error return";
+      LOG(ERROR)
+          << "Got duplicate id when commiting id: "
+          << local_entry.Get(ID)
+          << ". Treating as an error return";
       return CommitResponse::INVALID_MESSAGE;
     }
   }
@@ -230,7 +232,7 @@ ProcessCommitResponseCommand::ProcessSingleCommitResponse(
   }
 
   ProcessSuccessfulCommitResponse(commit_request_entry, server_entry,
-      pre_commit_id, &local_entry, syncing_was_set, deleted_folders);
+      local_entry.Get(ID), &local_entry, syncing_was_set, deleted_folders);
   return response;
 }
 
diff --git a/sync/engine/process_commit_response_command.h b/sync/engine/process_commit_response_command.h
index 155a47d..15ce4eb 100644
--- a/sync/engine/process_commit_response_command.h
+++ b/sync/engine/process_commit_response_command.h
@@ -70,7 +70,7 @@ class SYNC_EXPORT_PRIVATE ProcessCommitResponseCommand
       syncable::WriteTransaction* trans,
       const sync_pb::CommitResponse_EntryResponse& pb_commit_response,
       const sync_pb::SyncEntity& pb_committed_entry,
-      const syncable::Id& pre_commit_id,
+      int64 metahandle,
       std::set<syncable::Id>* deleted_folders);
 
   void ProcessSuccessfulCommitResponse(
diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc
index 1520ca2..7f3e369 100644
--- a/sync/engine/process_commit_response_command_unittest.cc
+++ b/sync/engine/process_commit_response_command_unittest.cc
@@ -98,7 +98,7 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 
     // ProcessCommitResponseCommand consumes commit_ids from the session
     // state, so we need to update that.  O(n^2) because it's a test.
-    commit_set->AddCommitItem(metahandle, item_id, model_type);
+    commit_set->AddCommitItem(metahandle, model_type);
 
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, syncable::GET_BY_ID, item_id);
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 96b664f..068d863 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -393,8 +393,8 @@ class SyncerTest : public testing::Test,
   }
 
   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) {
+                        const vector<int64>& expected_handle_order) {
+    for (size_t limit = expected_handle_order.size() + 2; limit > 0; --limit) {
       WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
 
       ModelSafeRoutingInfo routes;
@@ -407,10 +407,10 @@ class SyncerTest : public testing::Test,
                                    ModelTypeSet(), false,
                                    unsynced_handle_view, &ready_unsynced_set);
       command.BuildCommitIds(&wtrans, routes, ready_unsynced_set);
-      size_t truncated_size = std::min(limit, expected_id_order.size());
+      size_t truncated_size = std::min(limit, expected_handle_order.size());
       ASSERT_EQ(truncated_size, output_set.Size());
       for (size_t i = 0; i < truncated_size; ++i) {
-        ASSERT_EQ(expected_id_order[i], output_set.GetCommitIdAt(i))
+        ASSERT_EQ(expected_handle_order[i], output_set.GetCommitHandleAt(i))
             << "At index " << i << " with batch size limited to " << limit;
       }
       sessions::OrderedCommitSet::Projection proj;
@@ -418,10 +418,10 @@ class SyncerTest : public testing::Test,
       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 = output_set.GetCommitIdAt(proj[i]);
-        ASSERT_EQ(expected_id_order[proj[i]], projected);
+        int64 projected = output_set.GetCommitHandleAt(proj[i]);
+        ASSERT_EQ(expected_handle_order[proj[i]], projected);
         // Since this projection is the identity, the following holds.
-        ASSERT_EQ(expected_id_order[i], projected);
+        ASSERT_EQ(expected_handle_order[i], projected);
       }
     }
   }
@@ -600,6 +600,7 @@ TEST_F(SyncerTest, GetCommitIdsCommandTruncates) {
   CreateUnsyncedDirectory("E", ids_.MakeLocal("e"));
   CreateUnsyncedDirectory("J", ids_.MakeLocal("j"));
 
+  vector<int64> expected_order;
   {
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry_x(&wtrans, GET_BY_ID, ids_.MakeServer("x"));
@@ -620,24 +621,24 @@ TEST_F(SyncerTest, GetCommitIdsCommandTruncates) {
     entry_w.Put(SERVER_VERSION, 20);
     entry_w.Put(IS_UNAPPLIED_UPDATE, true);  // Fake a conflict.
     entry_j.PutPredecessor(entry_w.Get(ID));
+
+    // The expected order is "x", "b", "c", "d", "e", "j", truncated
+    // appropriately.
+    expected_order.push_back(entry_x.Get(META_HANDLE));
+    expected_order.push_back(entry_b.Get(META_HANDLE));
+    expected_order.push_back(entry_c.Get(META_HANDLE));
+    expected_order.push_back(entry_d.Get(META_HANDLE));
+    expected_order.push_back(entry_e.Get(META_HANDLE));
+    expected_order.push_back(entry_j.Get(META_HANDLE));
   }
 
   // The arrangement is now: x (b (d) c (e)) w j
   // Entry "w" is in conflict, so it is not eligible for commit.
   vector<int64> unsynced_handle_view;
-  vector<syncable::Id> expected_order;
   {
     syncable::ReadTransaction rtrans(FROM_HERE, directory());
     GetUnsyncedEntries(&rtrans, &unsynced_handle_view);
   }
-  // The expected order is "x", "b", "c", "d", "e", "j", truncated
-  // appropriately.
-  expected_order.push_back(ids_.MakeServer("x"));
-  expected_order.push_back(ids_.MakeLocal("b"));
-  expected_order.push_back(ids_.MakeLocal("c"));
-  expected_order.push_back(ids_.MakeLocal("d"));
-  expected_order.push_back(ids_.MakeLocal("e"));
-  expected_order.push_back(ids_.MakeLocal("j"));
   DoTruncationTest(unsynced_handle_view, expected_order);
 }