Fix handling of undeletion within the syncer.

Fix handling of undeletion within the syncer.

Accomplished by:
(a) Plumbing the const CommitMessage into process_commit_response_command.cc, so that we can know the server state better (IS_DEL is no longer authoritative if !
which spilled over into a couple test expectations.
(b) Relaxing the enforced invariants that version==0 implied !ServerKnows, for items with client tags.
(c) Use ID renaming instead of delete/recreate to handle collisions on the unique tag.
(d) Upon deletion, version number goes to 0 so that the server knows to process the item as a recreate, if it gets undeleted.  Previously, the server number would get synthesized on the client by incrementing the last known version number.
(e) Add unit tests for pretty much every ordering of delete/undelete I could come up with.

BUG=46769
TEST=unit tests; repeatedly uninstalled/installed extensions

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52209

[chron_chromium.org, 2010-07-12 21:30:41]

http://codereview.chromium.org/2844037/diff/11001/12001
File chrome/browser/sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12001#newcode95
chrome/browser/sync/engine/build_commit_command.cc:95:
SyncerProtoUtil::AddRequestBirthday(
Do we check this on the server side? I remember we had a longstanding bug where
the server doesn't verify this on commit.

http://codereview.chromium.org/2844037/diff/11001/12001#newcode154
chrome/browser/sync/engine/build_commit_command.cc:154: // Undeletions are only
correct for items that have a client tag.
Instead of 'correct' can you say "only supported" or something? Correctness
implies that we support incorrectly the other cases.

http://codereview.chromium.org/2844037/diff/11001/12002
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12002#newcode136
chrome/browser/sync/engine/conflict_resolver.cc:136: // If we've got a
client-unique tag, then the server supports simple
What's simple undeletion on the server?
It seems more like the client supports a different form of undeletion.

http://codereview.chromium.org/2844037/diff/11001/12002#newcode138
chrome/browser/sync/engine/conflict_resolver.cc:138:
DCHECK_EQ(entry.Get(syncable::SERVER_VERSION), 0);
Can you specify why this check is true? When does the version get set to zero?

http://codereview.chromium.org/2844037/diff/11001/12006
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12006#newcode123
chrome/browser/sync/engine/process_commit_response_command.cc:123: const
sync_pb::CommitMessage& commit_message = status->commit_message().commit();
80 col

http://codereview.chromium.org/2844037/diff/11001/12006#newcode309
chrome/browser/sync/engine/process_commit_response_command.cc:309: new_version =
0;
Can you add a comment here that expresses why the version must be set to zero? 
Specifically the invariant that !IS_UNAPPLIED_UPDATE && !IS_UNSYNCED means
versions must match.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode345
chrome/browser/sync/engine/process_commit_response_command.cc:345: // We should
trap this before this function.
TODO?

http://codereview.chromium.org/2844037/diff/11001/12006#newcode386
chrome/browser/sync/engine/process_commit_response_command.cc:386: //
TODO(nick): The server doesn't set entry_response.server_parent_id in
I just checked the server code and you're right. I'm surprised we don't set
this, but I guess changing the ID of the actual returned item causes this to be
reset as well, meaning that it's unecessary to send the new id back.

At some point we should just send back a full entity, but the server doesn't
support this.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode476
chrome/browser/sync/engine/process_commit_response_command.cc:476:
OverrideClientFieldsAfterCommit(committed_entry, entry_response,
awesome, much cleaner, thanks.

http://codereview.chromium.org/2844037/diff/11001/12012
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12012#newcode175
chrome/browser/sync/engine/syncer_util.cc:175: // This will result in an server
ID with base version == 0, but that's
Can you add a note about what happens to the server entry after this?

(server_entry is from the PB right now, and will end up applied into
local_entry's server half I think?)

http://codereview.chromium.org/2844037/diff/11001/12012#newcode210
chrome/browser/sync/engine/syncer_util.cc:210:
DCHECK(!server_entry.has_client_defined_unique_tag());
is this true? I think we have the capability to store originator data on the
server

http://codereview.chromium.org/2844037/diff/11001/12012#newcode371
chrome/browser/sync/engine/syncer_util.cc:371: local_entry->Put(SERVER_VERSION,
Why can't we set it always to zero?

http://codereview.chromium.org/2844037/diff/11001/12014
File chrome/browser/sync/engine/verify_updates_command.cc (left):

http://codereview.chromium.org/2844037/diff/11001/12014#oldcode97
chrome/browser/sync/engine/verify_updates_command.cc:97: if
(!entry.parent_id().ServerKnows()) {
When can the entry's parent be server unknown in an update?

http://codereview.chromium.org/2844037/diff/11001/12019
File chrome/browser/sync/syncable/syncable_id.h (left):

http://codereview.chromium.org/2844037/diff/11001/12019#oldcode102
chrome/browser/sync/syncable/syncable_id.h:102: public:
these were EXTRA PUBLIC :D

[ncarter, 2010-07-13 18:12:46]

http://codereview.chromium.org/2844037/diff/11001/12001
File chrome/browser/sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12001#newcode95
chrome/browser/sync/engine/build_commit_command.cc:95:
SyncerProtoUtil::AddRequestBirthday(
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Do we check this on the server side? I remember we had a longstanding bug
where
> the server doesn't verify this on commit.

It's irrelevant to this change; this code motion is because I wanted PostMessage
to take a const protobuf, so I moved all the AddRequestBirthdays to the callers.

http://codereview.chromium.org/2844037/diff/11001/12001#newcode154
chrome/browser/sync/engine/build_commit_command.cc:154: // Undeletions are only
correct for items that have a client tag.
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Instead of 'correct' can you say "only supported" or something? Correctness
> implies that we support incorrectly the other cases.

Done.

http://codereview.chromium.org/2844037/diff/11001/12002
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12002#newcode136
chrome/browser/sync/engine/conflict_resolver.cc:136: // If we've got a
client-unique tag, then the server supports simple
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> What's simple undeletion on the server?
> It seems more like the client supports a different form of undeletion.

Done.

http://codereview.chromium.org/2844037/diff/11001/12002#newcode138
chrome/browser/sync/engine/conflict_resolver.cc:138:
DCHECK_EQ(entry.Get(syncable::SERVER_VERSION), 0);
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Can you specify why this check is true? When does the version get set to zero?

Done.

http://codereview.chromium.org/2844037/diff/11001/12006
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12006#newcode123
chrome/browser/sync/engine/process_commit_response_command.cc:123: const
sync_pb::CommitMessage& commit_message = status->commit_message().commit();
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> 80 col

Done.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode309
chrome/browser/sync/engine/process_commit_response_command.cc:309: new_version =
0;
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Can you add a comment here that expresses why the version must be set to zero?

> Specifically the invariant that !IS_UNAPPLIED_UPDATE && !IS_UNSYNCED means
> versions must match.

Done.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode345
chrome/browser/sync/engine/process_commit_response_command.cc:345: // We should
trap this before this function.
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> TODO?

I don't think so -- this is old code, and I think the comment means "this ought
to have been caught earlier in the processing of the commit response".  It's the
duplicate-ID-return-treated-as-error case.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode386
chrome/browser/sync/engine/process_commit_response_command.cc:386: //
TODO(nick): The server doesn't set entry_response.server_parent_id in
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> I just checked the server code and you're right. I'm surprised we don't set
> this, but I guess changing the ID of the actual returned item causes this to
be
> reset as well, meaning that it's unecessary to send the new id back.
> 
> At some point we should just send back a full entity, but the server doesn't
> support this.

Done.

http://codereview.chromium.org/2844037/diff/11001/12006#newcode476
chrome/browser/sync/engine/process_commit_response_command.cc:476:
OverrideClientFieldsAfterCommit(committed_entry, entry_response,
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> awesome, much cleaner, thanks.

Done.

http://codereview.chromium.org/2844037/diff/11001/12012
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/2844037/diff/11001/12012#newcode175
chrome/browser/sync/engine/syncer_util.cc:175: // This will result in an server
ID with base version == 0, but that's
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Can you add a note about what happens to the server entry after this?
> 
> (server_entry is from the PB right now, and will end up applied into
> local_entry's server half I think?)

Done.

http://codereview.chromium.org/2844037/diff/11001/12012#newcode210
chrome/browser/sync/engine/syncer_util.cc:210:
DCHECK(!server_entry.has_client_defined_unique_tag());
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> is this true? I think we have the capability to store originator data on the
> server

Right you are.  Removed.

http://codereview.chromium.org/2844037/diff/11001/12012#newcode371
chrome/browser/sync/engine/syncer_util.cc:371: local_entry->Put(SERVER_VERSION,
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> Why can't we set it always to zero?

Mostly, I was trying to limit the scope of the invariant relaxation, since the
!ServerKnows checks had been historically useful in finding bugs.

http://codereview.chromium.org/2844037/diff/11001/12014
File chrome/browser/sync/engine/verify_updates_command.cc (left):

http://codereview.chromium.org/2844037/diff/11001/12014#oldcode97
chrome/browser/sync/engine/verify_updates_command.cc:97: if
(!entry.parent_id().ServerKnows()) {
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> When can the entry's parent be server unknown in an update?

It can't, which is why I removed the check.  The id() and parent_id() functions
on syncproto.h!SyncEntity always return server IDs.  This check was not bogus
when IDs were numeric, but it is bogus now.

http://codereview.chromium.org/2844037/diff/11001/12019
File chrome/browser/sync/syncable/syncable_id.h (left):

http://codereview.chromium.org/2844037/diff/11001/12019#oldcode102
chrome/browser/sync/syncable/syncable_id.h:102: public:
On 2010/07/12 21:30:41, chron_chromium.org wrote:
> these were EXTRA PUBLIC :D

Done.

[chron, 2010-07-13 18:20:41]

LGTM