Fix handling of undeletion within the syncer.

chrome/browser/sync/engine/conflict_resolver.cc

 // Copyright (c) 2009 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 "chrome/browser/sync/engine/conflict_resolver.h"
 
 #include <map>
 #include <set>
 
 #include "chrome/browser/sync/engine/syncer.h"
@@ skipping 77 matching lines @@
     // we've both deleted it, so lets just drop the need to commit/update this
     // entry.
     entry.Put(syncable::IS_UNSYNCED, false);
     entry.Put(syncable::IS_UNAPPLIED_UPDATE, false);
     // we've made changes, but they won't help syncing progress.
     // METRIC simple conflict resolved by merge.
     return NO_SYNC_PROGRESS;
   }
 
   if (!entry.Get(syncable::SERVER_IS_DEL)) {
-    // TODO(chron): Should we check more fields? Since IS_UNSYNCED is
-    // turned on, this is really probably enough as fields will be overwritten.
-    // Check if there's no changes.
-
-    // Verbose but easier to debug.
+    // This logic determines "client wins" vs. "server wins" strategy picking.
+    // TODO(nick): The current logic is arbitrary; instead, it ought to be made
+    // consistent with the ModelAssociator behavior for a datatype.  It would
+    // be nice if we could route this back to ModelAssociator code to pick one
+    // of three options: CLIENT, SERVER, or MERGE.  Some datatypes (autofill)
+    // are easily mergeable.
     bool name_matches = entry.Get(syncable::NON_UNIQUE_NAME) ==
                         entry.Get(syncable::SERVER_NON_UNIQUE_NAME);
     bool parent_matches = entry.Get(syncable::PARENT_ID) ==
                                     entry.Get(syncable::SERVER_PARENT_ID);
     bool entry_deleted = entry.Get(syncable::IS_DEL);
 
     if (!entry_deleted && name_matches && parent_matches) {
       LOG(INFO) << "Resolving simple conflict, ignoring local changes for:"
                 << entry;
       IgnoreLocalChanges(&entry);
@@ skipping 10 matching lines @@
       trans->directory()->GetChildHandles(trans,
                                           entry.Get(syncable::ID),
                                           &children);
       if (0 != children.size()) {
         LOG(INFO) << "Entry is a server deleted directory with local contents, "
           "should be in a set. (race condition).";
         return NO_SYNC_PROGRESS;
       }
     }
 
-    // If the entry's deleted on the server, we can have a directory here.
-    entry.Put(syncable::IS_UNSYNCED, true);
+    // The entry is deleted on the server but still exists locally.
+    if (!entry.Get(syncable::UNIQUE_CLIENT_TAG).empty()) {
+      // If we've got a client-unique tag, we can undelete while retaining
+      // our present ID.
+      DCHECK_EQ(entry.Get(syncable::SERVER_VERSION), 0)
+          << "For the server to know to re-create, client-tagged items should "
+          << "revert to version 0 when server-deleted.";
+      OverwriteServerChanges(trans, &entry);
+      // Clobber the versions, just in case the above DCHECK is violated.
+      entry.Put(syncable::SERVER_VERSION, 0);
+      entry.Put(syncable::BASE_VERSION, 0);
+    } else {
+      // Otherwise, we've got to undelete by creating a new locally
+      // uncommitted entry.
+      SyncerUtil::SplitServerInformationIntoNewEntry(trans, &entry);
 
-    SyncerUtil::SplitServerInformationIntoNewEntry(trans, &entry);
-
-    MutableEntry server_update(trans, syncable::GET_BY_ID, id);
-    CHECK(server_update.good());
-    CHECK(server_update.Get(syncable::META_HANDLE) !=
-          entry.Get(syncable::META_HANDLE))
-        << server_update << entry;
-
+      MutableEntry server_update(trans, syncable::GET_BY_ID, id);
+      CHECK(server_update.good());
+      CHECK(server_update.Get(syncable::META_HANDLE) !=
+            entry.Get(syncable::META_HANDLE))
+          << server_update << entry;
+    }
     return SYNC_PROGRESS;
   }
 }
 
 ConflictResolver::ConflictSetCountMapKey ConflictResolver::GetSetKey(
     ConflictSet* set) {
   // TODO(sync): Come up with a better scheme for set hashing. This scheme
   // will make debugging easy.
   // If this call to sort is removed, we need to add one before we use
   // binary_search in ProcessConflictSet.
@@ skipping 84 matching lines @@
     SyncerUtil::ChangeEntryIDAndUpdateChildren(trans, &parent,
         trans->directory()->NextId());
     parent.Put(syncable::BASE_VERSION, 0);
     parent.Put(syncable::IS_UNSYNCED, true);
     id = parent.Get(syncable::PARENT_ID);
     // METRIC conflict resolved by recreating dir tree.
   }
   return true;
 }
 
-
 // TODO(chron): needs unit test badly
 bool AttemptToFixUpdateEntryInDeletedLocalTree(WriteTransaction* trans,
                                                ConflictSet* conflict_set,
                                                const Entry& entry) {
   if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE) ||
       entry.Get(syncable::SERVER_IS_DEL))
     return false;
   Id parent_id = entry.Get(syncable::SERVER_PARENT_ID);
   MutableEntry parent(trans, syncable::GET_BY_ID, parent_id);
   if (!parent.good() || !parent.Get(syncable::IS_DEL) ||
@@ skipping 241 matching lines @@
       conflict_set_count_map_.erase(i++);
       // METRIC self resolved conflict sets ++.
     } else {
       ++i;
     }
   }
   return rv;
 }
 
 }  // namespace browser_sync