Fix GetCommitIds local deletion case resulting in orphaning of directory entries

Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and
verifies the fix.

BUG=569775
Committed: https://crrev.com/15dfe4497acc9320e7453e38edec62acbf237716
Cr-Commit-Position: refs/heads/master@{#367565}

[stanisc, 2015-12-29 18:25:33]

Patchset #3 (id:40001) has been deleted

[stanisc, 2015-12-29 20:11:14]

Description was changed from

==========
Test for deletion conflict

BUG=569775
==========

to

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering 
part of the algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

[stanisc, 2015-12-29 20:12:43]

Description was changed from

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering 
part of the algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

to

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

[stanisc, 2015-12-29 20:13:41]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2015-12-29 20:13:41]

PTAL!

[Nicolas Zea, 2015-12-31 02:12:03]

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:197: for (std::vector<int64_t>::const_iterator
iter =
nit: use auto? (here and elsewhere)

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:223: // Excludes children of items from
|conflicted_items| list.
Mention excluded_items output as well?

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:297: // If there are any deleted conflicted
entries, remote their deleted ancestors
nit: remote -> remove

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:2875: // orphan the nested folder.
Is there any test that verifies that children of conflicting folders aren't
committed?

[stanisc, 2016-01-04 22:00:48]

Addressed CR feedback. PTAL!

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:197: for (std::vector<int64_t>::const_iterator
iter =
On 2015/12/31 02:12:02, Nicolas Zea wrote:
> nit: use auto? (here and elsewhere)

Done.

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:223: // Excludes children of items from
|conflicted_items| list.
On 2015/12/31 02:12:02, Nicolas Zea wrote:
> Mention excluded_items output as well?

Done.

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/get_commit_...
sync/engine/get_commit_ids.cc:297: // If there are any deleted conflicted
entries, remote their deleted ancestors
On 2015/12/31 02:12:03, Nicolas Zea wrote:
> nit: remote -> remove

Done.

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/1548843002/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:2875: // orphan the nested folder.
On 2015/12/31 02:12:03, Nicolas Zea wrote:
> Is there any test that verifies that children of conflicting folders aren't
> committed?

I think there are some tests that indirectly verify that already because I
remember running into the commit IDs filtering code before. But since it wasn't
clear which tests verify that I decided it would be a good idea to add a new one
- CommittingItemsWithConflictInParentFolder.

[Nicolas Zea, 2016-01-05 01:35:25]

LGTM!

[stanisc, 2016-01-05 17:41:43]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2016-01-05 17:42:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1548843002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1548843002/80001

[commit-bot: I haz the power, 2016-01-05 17:49:22]

Description was changed from

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

to

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

[commit-bot: I haz the power, 2016-01-05 17:49:22]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-01-05 17:50:11]

Description was changed from

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and 
verifies the fix.

BUG=569775
==========

to

==========
Fix GetCommitIds local deletion case resulting in orphaning of directory entries

This fixes filtering of commits in the case when a hierarchy of bookmarks
gets deleted and there is a conflict in the middle of the hierarchy.
Before the fix the folder above the conflicting entry would be still
committed and then subsequently deleted and that would leave the conflicting
entry orphaned and unreachable on both the clint and the server.

The fix provides a better implementation of FilterUnreadyEntries which
removes not only entries that are in direct conflict themselves but also all
other entries dependent on them. For conflicting deleted entries it removes
all deleted folders above them. For conflicting non-deleted folders it removes
all entries below (prior to the fix that was done in the ordering part of the
algorithm).

I've included a new unit test that reproduces the bug condition and
verifies the fix.

BUG=569775

Committed: https://crrev.com/15dfe4497acc9320e7453e38edec62acbf237716
Cr-Commit-Position: refs/heads/master@{#367565}
==========

[commit-bot: I haz the power, 2016-01-05 17:50:12]

Patchset 4 (id:??) landed as
https://crrev.com/15dfe4497acc9320e7453e38edec62acbf237716
Cr-Commit-Position: refs/heads/master@{#367565}