Sync: set BASE_VERSION to entry.version() instead of 0 when changing

Sync: set BASE_VERSION to entry.version() instead of 0 when changing
IDs, but also force IS_UNAPPLIED.

This allows BV to be valid in case the item is marked UNSYNCED and later commits,
but still allows the update application to proceed if it's not.

BUG=49715
TEST=See bug

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

[ncarter (slow), 2010-07-27 21:39:09]

Still doing manual testing on this, but it looks good so far.

[chron_chromium.org, 2010-07-27 22:36:53]

Is the spurious commit really OK? Won't the commit happen with the latest
version as BV, causing overwrite?

On Tue, Jul 27, 2010 at 2:39 PM, <nick@chromium.org> wrote:

> Reviewers: chron_chromium.org,
>
> Message:
> Still doing manual testing on this, but it looks good so far.
>
> Description:
> Sync: set BASE_VERSION to entry.version() instead of 0 when changing
> IDs, but also force IS_UNAPPLIED.
>
> This allows BV to be valid in case the item is marked UNSYNCED and later
> commits,
> but still allows the update application to proceed if it's not.
>
> BUG=49715
> TEST=See bug
>
> Please review this at http://codereview.chromium.org/3054021/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
>  M chrome/browser/sync/engine/process_updates_command.cc
>  M chrome/browser/sync/engine/syncer_unittest.cc
>
>
>

[ncarter (slow), 2010-07-27 23:05:27]

I think this is the safest behavior for M6.  Before, we'd do a spurious
commit that wound up creating a duplicate object.  Now we'll do a commit
that's a legitimate edit of the existing object.  The result should be akin
to what would happen if the conflict resolver picked the local version.
 UpdateServerFieldsAfterCommit will clear the IS_UNAPPLIED_UPDATE field.  I
regard this as kind of a separate bug: we shouldn't allow UNAPPLIED_UPDATE
items into the commit set to begin with.

The simplest alternative that allows the conflict resolver to actually
trigger, is to use max(1, entry.version() - 1).  I wanted to avoid this
mostly because it yields an inconsistent behavior when version is exactly 1;
one of the unit tests does exercise exactly that case, though I think it
won't happen in practice.

Let me know if you prefer the decremented version number.  I could go either
way.

 - nick

On Tue, Jul 27, 2010 at 3:36 PM, Jonathan Huang <chron@chromium.org> wrote:

> Is the spurious commit really OK? Won't the commit happen with the latest
> version as BV, causing overwrite?
>
>
> On Tue, Jul 27, 2010 at 2:39 PM, <nick@chromium.org> wrote:
>
>> Reviewers: chron_chromium.org,
>>
>> Message:
>> Still doing manual testing on this, but it looks good so far.
>>
>> Description:
>> Sync: set BASE_VERSION to entry.version() instead of 0 when changing
>> IDs, but also force IS_UNAPPLIED.
>>
>> This allows BV to be valid in case the item is marked UNSYNCED and later
>> commits,
>> but still allows the update application to proceed if it's not.
>>
>> BUG=49715
>> TEST=See bug
>>
>> Please review this at http://codereview.chromium.org/3054021/show
>>
>> SVN Base: http://src.chromium.org/git/chromium.git
>>
>> Affected files:
>>  M chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
>>  M chrome/browser/sync/engine/process_updates_command.cc
>>  M chrome/browser/sync/engine/syncer_unittest.cc
>>
>>
>>
>

[chron_chromium.org, 2010-07-27 23:07:59]

LGTM

But please document the spurious commit case as a comment. It will result in
an overwrite. (Which I don't think will loop)



On Tue, Jul 27, 2010 at 4:05 PM, Nick Carter <nick@chromium.org> wrote:

> I think this is the safest behavior for M6.  Before, we'd do a spurious
> commit that wound up creating a duplicate object.  Now we'll do a commit
> that's a legitimate edit of the existing object.  The result should be akin
> to what would happen if the conflict resolver picked the local version.
>  UpdateServerFieldsAfterCommit will clear the IS_UNAPPLIED_UPDATE field.  I
> regard this as kind of a separate bug: we shouldn't allow UNAPPLIED_UPDATE
> items into the commit set to begin with.
>
> The simplest alternative that allows the conflict resolver to actually
> trigger, is to use max(1, entry.version() - 1).  I wanted to avoid this
> mostly because it yields an inconsistent behavior when version is exactly 1;
> one of the unit tests does exercise exactly that case, though I think it
> won't happen in practice.
>
> Let me know if you prefer the decremented version number.  I could go
> either way.
>
>  - nick
>
>
> On Tue, Jul 27, 2010 at 3:36 PM, Jonathan Huang <chron@chromium.org>wrote:
>
>> Is the spurious commit really OK? Won't the commit happen with the latest
>> version as BV, causing overwrite?
>>
>>
>> On Tue, Jul 27, 2010 at 2:39 PM, <nick@chromium.org> wrote:
>>
>>> Reviewers: chron_chromium.org,
>>>
>>> Message:
>>> Still doing manual testing on this, but it looks good so far.
>>>
>>> Description:
>>> Sync: set BASE_VERSION to entry.version() instead of 0 when changing
>>> IDs, but also force IS_UNAPPLIED.
>>>
>>> This allows BV to be valid in case the item is marked UNSYNCED and later
>>> commits,
>>> but still allows the update application to proceed if it's not.
>>>
>>> BUG=49715
>>> TEST=See bug
>>>
>>> Please review this at http://codereview.chromium.org/3054021/show
>>>
>>> SVN Base: http://src.chromium.org/git/chromium.git
>>>
>>> Affected files:
>>>  M chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
>>>  M chrome/browser/sync/engine/process_updates_command.cc
>>>  M chrome/browser/sync/engine/syncer_unittest.cc
>>>
>>>
>>>
>>
>

[ncarter (slow), 2010-07-27 23:51:29]

On 2010/07/27 23:07:59, chron_chromium.org wrote:
> But please document the spurious commit case as a comment. It will result in
> an overwrite. (Which I don't think will loop)

Done, and I manually tested this with an autofill conflict; it doesn't loop.