Remove single direction conflict set code

This code was intended to operate on conflict sets that consist solely
of changes from the server.  However, we can never detect these kinds of
conflict sets because our conflict set detection code assumes that the
server never sends us an invalid set of changes; all conflict sets must
be cause, at least in part, by local changes.

This change also deletes various private and hidden functions that were
used only by the single direction conflict set code.  This reduced the
BuildAndProcessConflictSetsCommand::ExecuteImpl() into a single function
call, which I inlined.

Also removed in this change is the conflict_sets_built() flag.  The
single direction conflict set processing code was the only place where
its value was "set".  It seems that the logic that relied on it in
syncer.cc was reversed.  We only processed conflicts if
!conflict_sets_built(), which is the opposite of what the documentation
claims.  Fortunately, its value was always false (because there were no
single direction conflict sets) so we always processed conflicts.

Finally, this CL includes one unrelated change: it removes
BuildAndProcessConflictSetsCommand::MergeSetsForNameClash().  That
function is not defined anywhere, so I decided to remove its
declaration.

BUG=107816
TEST=


Review URL: http://codereview.chromium.org/9107055

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

9 files changed, 8 insertions, 235 deletions

diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
index b4fc41e..3db89f0 100644
--- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
+++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
@@ -37,198 +37,16 @@ std::set<ModelSafeGroup> BuildAndProcessConflictSetsCommand::GetGroupsToChange(
 
 SyncerError BuildAndProcessConflictSetsCommand::ModelChangingExecuteImpl(
     SyncSession* session) {
-  session->mutable_status_controller()->update_conflict_sets_built(
-      BuildAndProcessConflictSets(session));
-  return SYNCER_OK;
-}
-
-bool BuildAndProcessConflictSetsCommand::BuildAndProcessConflictSets(
-    SyncSession* session) {
   syncable::ScopedDirLookup dir(session->context()->directory_manager(),
                                 session->context()->account_name());
   if (!dir.good())
-    return false;
-  bool had_single_direction_sets = false;
-  {  // Scope for transaction.
-    syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
-    BuildConflictSets(&trans,
-        session->mutable_status_controller()->mutable_conflict_progress());
-    had_single_direction_sets = ProcessSingleDirectionConflictSets(&trans,
-        session->context()->resolver(),
-        session->context()->directory_manager()->GetCryptographer(&trans),
-        session->mutable_status_controller(), session->routing_info());
-    // We applied some updates transactionally, lets try syncing again.
-    if (had_single_direction_sets)
-      return true;
-  }
-  return false;
-}
-
-bool BuildAndProcessConflictSetsCommand::ProcessSingleDirectionConflictSets(
-    syncable::WriteTransaction* trans, ConflictResolver* resolver,
-    Cryptographer* cryptographer, StatusController* status,
-    const ModelSafeRoutingInfo& routes) {
-  if (!status->conflict_progress())
-    return false;
-  bool rv = false;
-  set<ConflictSet*>::const_iterator all_sets_iterator;
-  for (all_sets_iterator = status->conflict_progress()->ConflictSetsBegin();
-       all_sets_iterator != status->conflict_progress()->ConflictSetsEnd();) {
-    const ConflictSet* conflict_set = *all_sets_iterator;
-    CHECK_GE(conflict_set->size(), 2U);
-    // We scan the set to see if it consists of changes of only one type.
-    ConflictSet::const_iterator i;
-    size_t unsynced_count = 0, unapplied_count = 0;
-    for (i = conflict_set->begin(); i != conflict_set->end(); ++i) {
-      syncable::Entry entry(trans, syncable::GET_BY_ID, *i);
-      CHECK(entry.good());
-      if (entry.Get(syncable::IS_UNSYNCED))
-        unsynced_count++;
-      if (entry.Get(syncable::IS_UNAPPLIED_UPDATE))
-        unapplied_count++;
-    }
-    if (conflict_set->size() == unsynced_count && 0 == unapplied_count) {
-      DVLOG(1) << "Skipped transactional commit attempt.";
-    } else if (conflict_set->size() == unapplied_count && 0 == unsynced_count &&
-          ApplyUpdatesTransactionally(trans, conflict_set, resolver,
-                                      cryptographer, routes, status)) {
-      rv = true;
-    }
-    ++all_sets_iterator;
-  }
-  return rv;
-}
-
-namespace {
-
-void StoreLocalDataForUpdateRollback(syncable::Entry* entry,
-                                     syncable::EntryKernel* backup) {
-  CHECK(!entry->Get(syncable::IS_UNSYNCED)) << " Storing Rollback data for "
-      "entry that's unsynced." << *entry;
-  CHECK(entry->Get(syncable::IS_UNAPPLIED_UPDATE)) << " Storing Rollback data "
-      "for entry that's not an unapplied update." << *entry;
-  *backup = entry->GetKernelCopy();
-}
-
+    return DIRECTORY_LOOKUP_FAILED;
 
-bool RollbackEntry(syncable::WriteTransaction* trans,
-                   syncable::EntryKernel* backup) {
-  syncable::MutableEntry entry(trans, syncable::GET_BY_HANDLE,
-                               backup->ref(syncable::META_HANDLE));
-  CHECK(entry.good());
-
-  if (!entry.Put(syncable::IS_DEL, backup->ref(syncable::IS_DEL)))
-    return false;
-
-  entry.Put(syncable::NON_UNIQUE_NAME, backup->ref(syncable::NON_UNIQUE_NAME));
-  entry.Put(syncable::PARENT_ID, backup->ref(syncable::PARENT_ID));
-
-  if (!backup->ref(syncable::IS_DEL)) {
-    if (!entry.PutPredecessor(backup->ref(syncable::PREV_ID))) {
-      // TODO(lipalani) : Propagate the error to caller. crbug.com/100444.
-      NOTREACHED();
-    }
-  }
+  syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
+  BuildConflictSets(&trans,
+      session->mutable_status_controller()->mutable_conflict_progress());
 
-  if (backup->ref(syncable::PREV_ID) != entry.Get(syncable::PREV_ID))
-    return false;
-
-  entry.Put(syncable::CTIME, backup->ref(syncable::CTIME));
-  entry.Put(syncable::MTIME, backup->ref(syncable::MTIME));
-  entry.Put(syncable::BASE_VERSION, backup->ref(syncable::BASE_VERSION));
-  entry.Put(syncable::IS_DIR, backup->ref(syncable::IS_DIR));
-  entry.Put(syncable::IS_DEL, backup->ref(syncable::IS_DEL));
-  entry.Put(syncable::ID, backup->ref(syncable::ID));
-  entry.Put(syncable::IS_UNAPPLIED_UPDATE,
-            backup->ref(syncable::IS_UNAPPLIED_UPDATE));
-  return true;
-}
-
-void PlaceEntriesAtRoot(syncable::WriteTransaction* trans,
-                      const vector<syncable::Id>* ids) {
-    vector<syncable::Id>::const_iterator it;
-    for (it = ids->begin(); it != ids->end(); ++it) {
-      syncable::MutableEntry entry(trans, syncable::GET_BY_ID, *it);
-    entry.Put(syncable::PARENT_ID, trans->root_id());
-    }
-  }
-
-}  // namespace
-
-bool BuildAndProcessConflictSetsCommand::ApplyUpdatesTransactionally(
-    syncable::WriteTransaction* trans,
-    const vector<syncable::Id>* const update_set,
-    ConflictResolver* resolver,
-    Cryptographer* cryptographer,
-    const ModelSafeRoutingInfo& routes,
-    StatusController* status) {
-  // The handles in the |update_set| order.
-  vector<int64> handles;
-
-  // Holds the same Ids as update_set, but sorted so that runs of adjacent
-  // nodes appear in order.
-  vector<syncable::Id> rollback_ids;
-  rollback_ids.reserve(update_set->size());
-
-  // Tracks what's added to |rollback_ids|.
-  syncable::MetahandleSet rollback_ids_inserted_items;
-  vector<syncable::Id>::const_iterator it;
-
-  // 1. Build |rollback_ids| in the order required for successful rollback.
-  //    Specifically, for positions to come out right, restoring an item
-  //    requires that its predecessor in the sibling order is properly
-  //    restored first.
-  // 2. Build |handles|, the list of handles for ApplyUpdates.
-  for (it = update_set->begin(); it != update_set->end(); ++it) {
-    syncable::Entry entry(trans, syncable::GET_BY_ID, *it);
-    SyncerUtil::AddPredecessorsThenItem(trans, &entry,
-        syncable::IS_UNAPPLIED_UPDATE, &rollback_ids_inserted_items,
-        &rollback_ids);
-    handles.push_back(entry.Get(syncable::META_HANDLE));
-  }
-  DCHECK_EQ(rollback_ids.size(), update_set->size());
-  DCHECK_EQ(rollback_ids_inserted_items.size(), update_set->size());
-
-  // 3. Store the information needed to rollback if the transaction fails.
-  // Do this before modifying anything to keep the next/prev values intact.
-  vector<syncable::EntryKernel> rollback_data(rollback_ids.size());
-  for (size_t i = 0; i < rollback_ids.size(); ++i) {
-    syncable::Entry entry(trans, syncable::GET_BY_ID, rollback_ids[i]);
-    StoreLocalDataForUpdateRollback(&entry, &rollback_data[i]);
-  }
-
-  // 4. Use the preparer to move things to an initial starting state where
-  // nothing in the set is a child of anything else.  If
-  // we've correctly calculated the set, the server tree is valid and no
-  // changes have occurred locally we should be able to apply updates from this
-  // state.
-  PlaceEntriesAtRoot(trans, update_set);
-
-  // 5. Use the usual apply updates from the special start state we've just
-  // prepared.
-  UpdateApplicator applicator(resolver, cryptographer,
-                              handles.begin(), handles.end(),
-                              routes, status->group_restriction());
-  while (applicator.AttemptOneApplication(trans)) {
-    // Keep going till all updates are applied.
-  }
-  if (!applicator.AllUpdatesApplied()) {
-    LOG(ERROR) << "Transactional Apply Failed, Rolling back.";
-    // We have to move entries into the temp dir again. e.g. if a swap was in a
-    // set with other failing updates, the swap may have gone through, meaning
-    // the roll back needs to be transactional. But as we're going to a known
-    // good state we should always succeed.
-    PlaceEntriesAtRoot(trans, update_set);
-
-    // Rollback all entries.
-    for (size_t i = 0; i < rollback_data.size(); ++i) {
-      CHECK(RollbackEntry(trans, &rollback_data[i]));
-    }
-    return false;  // Don't save progress -- we just undid it.
-  }
-  applicator.SaveProgressIntoSessionState(status->mutable_conflict_progress(),
-                                          status->mutable_update_progress());
-  return true;
+  return SYNCER_OK;
 }
 
 void BuildAndProcessConflictSetsCommand::BuildConflictSets(
diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h
index 1ae090d..99f130d 100644
--- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h
+++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h
@@ -43,25 +43,9 @@ class BuildAndProcessConflictSetsCommand : public ModelChangingSyncerCommand {
       sessions::SyncSession* session) OVERRIDE;
 
  private:
-  bool BuildAndProcessConflictSets(sessions::SyncSession* session);
-
-  bool ProcessSingleDirectionConflictSets(
-      syncable::WriteTransaction* trans, ConflictResolver* resolver,
-      Cryptographer* cryptographer, sessions::StatusController* status,
-      const ModelSafeRoutingInfo& routes);
-  bool ApplyUpdatesTransactionally(
-      syncable::WriteTransaction* trans,
-      const std::vector<syncable::Id>* const update_set,
-      ConflictResolver* resolver,
-      Cryptographer* cryptographer,
-      const ModelSafeRoutingInfo& routes,
-      sessions::StatusController* status);
   void BuildConflictSets(syncable::BaseTransaction* trans,
                          sessions::ConflictProgress* conflict_progress);
 
-  void MergeSetsForNameClash(syncable::BaseTransaction* trans,
-                             syncable::Entry* entry,
-                             sessions::ConflictProgress* conflict_progress);
   void MergeSetsForIntroducedLoops(syncable::BaseTransaction* trans,
       syncable::Entry* entry,
       sessions::ConflictProgress* conflict_progress);
diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc
index fe2a49c..dbab2fb 100644
--- a/chrome/browser/sync/engine/syncer.cc
+++ b/chrome/browser/sync/engine/syncer.cc
@@ -240,10 +240,7 @@ void Syncer::SyncShare(sessions::SyncSession* session,
       case BUILD_AND_PROCESS_CONFLICT_SETS: {
         BuildAndProcessConflictSetsCommand build_process_conflict_sets;
         build_process_conflict_sets.Execute(session);
-        if (session->status_controller().conflict_sets_built())
-          next_step = SYNCER_END;
-        else
-          next_step = RESOLVE_CONFLICTS;
+        next_step = RESOLVE_CONFLICTS;
         break;
       }
       case RESOLVE_CONFLICTS: {
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index 4020b0c..ce1f462 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -258,14 +258,9 @@ class UpdateProgress {
 };
 
 struct SyncCycleControlParameters {
-  SyncCycleControlParameters() : conflict_sets_built(false),
-                                 conflicts_resolved(false),
+  SyncCycleControlParameters() : conflicts_resolved(false),
                                  items_committed(false),
                                  debug_info_sent(false) {}
-  // Set to true by BuildAndProcessConflictSetsCommand if the RESOLVE_CONFLICTS
-  // step is needed.
-  bool conflict_sets_built;
-
   // Set to true by ResolveConflictsCommand if any forward progress was made.
   bool conflicts_resolved;
 
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index 13371dd..95c8c4f 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -225,9 +225,6 @@ void StatusController::set_commit_set(const OrderedCommitSet& commit_set) {
   shared_.commit_set = commit_set;
 }
 
-void StatusController::update_conflict_sets_built(bool built) {
-  shared_.control_params.conflict_sets_built |= built;
-}
 void StatusController::update_conflicts_resolved(bool resolved) {
   shared_.control_params.conflicts_resolved |= resolved;
 }
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index 5e397e1..b072577 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -138,9 +138,6 @@ class StatusController {
   }
 
   // Control parameters for sync cycles.
-  bool conflict_sets_built() const {
-    return shared_.control_params.conflict_sets_built;
-  }
   bool conflicts_resolved() const {
     return shared_.control_params.conflicts_resolved;
   }
@@ -232,7 +229,6 @@ class StatusController {
   void set_last_process_commit_response_result(const SyncerError result);
 
   void set_commit_set(const OrderedCommitSet& commit_set);
-  void update_conflict_sets_built(bool built);
   void update_conflicts_resolved(bool resolved);
   void reset_conflicts_resolved();
   void set_items_committed();
diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc
index 2c7b692..1b3c17f 100644
--- a/chrome/browser/sync/sessions/status_controller_unittest.cc
+++ b/chrome/browser/sync/sessions/status_controller_unittest.cc
@@ -78,8 +78,6 @@ TEST_F(StatusControllerTest, GetsDirty) {
 
 TEST_F(StatusControllerTest, StaysClean) {
   StatusController status(routes_);
-  status.update_conflict_sets_built(true);
-  EXPECT_FALSE(status.TestAndClearIsDirty());
   status.update_conflicts_resolved(true);
   EXPECT_FALSE(status.TestAndClearIsDirty());
 
@@ -125,10 +123,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.update_conflicts_resolved(true);
   EXPECT_TRUE(status.conflicts_resolved());
 
-  EXPECT_FALSE(status.conflict_sets_built());
-  status.update_conflict_sets_built(true);
-  EXPECT_TRUE(status.conflict_sets_built());
-
   status.set_last_download_updates_result(SYNCER_OK);
   EXPECT_EQ(SYNCER_OK, status.error().last_download_updates_result);
 
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index b0b53b4..47ca7b1 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -189,7 +189,6 @@ bool SyncSession::HasMoreToSync() const {
   const StatusController* status = status_controller_.get();
   return ((status->commit_ids().size() < status->unsynced_handles().size()) &&
       status->syncer_status().num_successful_commits > 0) ||
-      status->conflict_sets_built() ||
       status->conflicts_resolved();
       // Or, we have conflicting updates, but we're making progress on
       // resolving them...
diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc
index 660729a..4ec6efc 100644
--- a/chrome/browser/sync/sessions/sync_session_unittest.cc
+++ b/chrome/browser/sync/sessions/sync_session_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -217,13 +217,6 @@ TEST_F(SyncSessionTest, MoreToSyncIfUnsyncedGreaterThanCommitted) {
   EXPECT_TRUE(session_->HasMoreToSync());
 }
 
-TEST_F(SyncSessionTest, MoreToSyncIfConflictSetsBuilt) {
-  // If we built conflict sets, then we need to loop back and try
-  // to get updates & commit again.
-  status()->update_conflict_sets_built(true);
-  EXPECT_TRUE(session_->HasMoreToSync());
-}
-
 TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
   status()->set_updates_request_types(ParamsMeaningAllEnabledTypes());