[SyncFS] Fix reaching NOTREACHED() in MetadataDatabase

[SyncFS] Fix reaching NOTREACHED() in MetadataDatabase::GetConflictingTrackers

Trackers for empty-title files were not handled in AddToPathIndexes()
and UpdateInPathIndexes(), while they are handled in RemoveFromPathIndexes().
So an entry in multi-backing lists could be inconsistent.

This CL fixes those two methods' behavior.


BUG=405448
TEST=./unit_tests --gtest_filter="MetadataDatabaseIndexOnDisk*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291186

[peria, 2014-08-20 09:10:24]

PTL.

Changes of metadata_database_index.cc in http://crrev.com/275363 may be a
reference.

[tzik, 2014-08-20 10:48:47]

lgtm

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:889:
GenerateMultiBackingParentAndTitleKey(parent_id, title);
It doesn't need to be fix in this CL, but GenerateMultiBackingParentAndTitleKey
sounds confusing.

MetadataDatabaseIndex has "index for file IDs that has multiple trackers" and
"index for a path (a pair of the parent tracker ID and the title) that has
multiple trackers".
Their names are |multi_tracker_file_ids_| and |multi_backing_file_path_|.

Can we rename them to MultiTrackerFile and MultiTrackerPath?
Also, comments at metadata_database_index_on_disk.cc:57 and :71 look inaccurate
on their types. Can you update it?

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:892:
db_->Put(GenerateMultiBackingParentAndTitleKey(parent_id, title),
can we reuse |multi_key|?

[nhiroki, 2014-08-21 03:03:15]

> Handling of trackers for empty-title files was not enough.
> So an entry in multi-backing lists may not have multiple trackers in other
lists.

Can you update the issue description to make it more descriptive? Why wasn't it
enough? What did you do in this CL for that?

[nhiroki, 2014-08-21 03:12:52]

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:899:
GenerateActiveTrackerIDByParentAndTitleKey(parent_id, title),
Just to confirm... is an empty title valid here?

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:939:
GenerateActiveTrackerIDByParentAndTitleKey(parent_id, title),
ditto.

[peria, 2014-08-21 06:25:40]

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc
(right):

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:889:
GenerateMultiBackingParentAndTitleKey(parent_id, title);
On 2014/08/20 10:48:47, tzik wrote:
> It doesn't need to be fix in this CL, but
GenerateMultiBackingParentAndTitleKey
> sounds confusing.
> 
> MetadataDatabaseIndex has "index for file IDs that has multiple trackers" and
> "index for a path (a pair of the parent tracker ID and the title) that has
> multiple trackers".
> Their names are |multi_tracker_file_ids_| and |multi_backing_file_path_|.
> 
> Can we rename them to MultiTrackerFile and MultiTrackerPath?
> Also, comments at metadata_database_index_on_disk.cc:57 and :71 look
inaccurate
> on their types. Can you update it?

Let me do it in another CL.
I think they are confusing, too.

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:892:
db_->Put(GenerateMultiBackingParentAndTitleKey(parent_id, title),
On 2014/08/20 10:48:47, tzik wrote:
> can we reuse |multi_key|?

Done.

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:899:
GenerateActiveTrackerIDByParentAndTitleKey(parent_id, title),
On 2014/08/21 03:12:52, nhiroki wrote:
> Just to confirm... is an empty title valid here?

Hmm, to be honest, I'm not sure about it.
MetadataDatabaseIndex accepts empty titles in |trackers_by_parent_and_title_|.

In unit tests, putting a place holder goes through this path with empty title.

https://codereview.chromium.org/493643002/diff/1/chrome/browser/sync_file_sys...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk.cc:939:
GenerateActiveTrackerIDByParentAndTitleKey(parent_id, title),
On 2014/08/21 03:12:52, nhiroki wrote:
> ditto.

ditto.

[nhiroki, 2014-08-21 06:53:28]

LGTM (I didn't take a close look at the test though)

https://codereview.chromium.org/493643002/diff/20001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc
(right):

https://codereview.chromium.org/493643002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc:488:

nit: blank lines

[peria, 2014-08-21 07:10:58]

https://codereview.chromium.org/493643002/diff/20001/chrome/browser/sync_file...
File
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc
(right):

https://codereview.chromium.org/493643002/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/metadata_database_index_on_disk_unittest.cc:488:

On 2014/08/21 06:53:28, nhiroki wrote:
> nit: blank lines

Done.

[peria, 2014-08-21 07:11:03]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-08-21 07:12:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/493643002/40001

[commit-bot: I haz the power, 2014-08-21 13:13:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-21 21:12:36]

Committed patchset #3 (40001) as 291186