From 943444a7c34653d0e3db009711d5b38a30c22b48 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Fri, 6 Nov 2015 13:32:21 +0300 Subject: [PATCH 1/3] [platform, storage] Implemented cleanup of absent countries indexes. --- map/framework.cpp | 6 ++- platform/local_country_file_utils.cpp | 43 ++++++++++++++++--- platform/local_country_file_utils.hpp | 8 +++- .../local_country_file_tests.cpp | 35 +++++++++++---- .../platform_tests_support/scoped_dir.cpp | 5 +++ .../platform_tests_support/scoped_dir.hpp | 2 + storage/storage.cpp | 14 +++--- storage/storage_tests/storage_tests.cpp | 27 ++++++++++++ 8 files changed, 117 insertions(+), 23 deletions(-) diff --git a/map/framework.cpp b/map/framework.cpp index 530cbcc67f..e003c9a7ec 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -477,7 +477,11 @@ void Framework::RegisterAllMaps() ("Registering maps while map downloading leads to removing downloading maps from " "ActiveMapsListener::m_items.")); - platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion()); + auto const isCountryName = [this](string const & filename) + { + return m_storage.FindIndexByFile(filename).IsValid(); + }; + platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion(), isCountryName); m_storage.RegisterAllLocalMaps(); int minFormat = numeric_limits::max(); diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 966d292a69..56525a9e0e 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -1,6 +1,8 @@ -#include "local_country_file_utils.hpp" -#include "mwm_version.hpp" -#include "platform.hpp" +#include "platform/local_country_file_utils.hpp" + +#include "platform/country_file.hpp" +#include "platform/mwm_version.hpp" +#include "platform/platform.hpp" #include "coding/file_name_utils.hpp" #include "coding/internal/file_data.hpp" @@ -15,6 +17,7 @@ #include "std/cctype.hpp" #include "std/sstream.hpp" #include "std/unique_ptr.hpp" +#include "std/unordered_set.hpp" #include "defines.hpp" @@ -82,6 +85,31 @@ void DeleteDownloaderFilesForAllCountries(string const & directory) for (auto const & file : files) my::DeleteFileX(my::JoinFoldersToPath(directory, file)); } + +void DeleteIndexesForAbsentCountries(string const & directory, + int64_t version, + function const & isCountryName) +{ + vector files; + FindAllLocalMapsInDirectory(directory, version, files); + + unordered_set names; + for (auto const & file : files) + names.insert(file.GetCountryName()); + + Platform::FilesList subdirs; + Platform::GetFilesByType(directory, Platform::FILE_TYPE_DIRECTORY, subdirs); + for (auto const & subdir : subdirs) + { + if (subdir == "." || subdir == "..") + continue; + if (!isCountryName(subdir) || names.count(subdir) != 0) + continue; + + LocalCountryFile absentCountry(directory, CountryFile(subdir), version); + CountryIndexes::DeleteFromDisk(absentCountry); + } +} } // namespace void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version) @@ -96,7 +124,8 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve } } -void CleanupMapsDirectory(int64_t latestVersion) +void CleanupMapsDirectory(int64_t latestVersion, + function const & isCountryName) { Platform & platform = GetPlatform(); @@ -137,6 +166,9 @@ void CleanupMapsDirectory(int64_t latestVersion) if (version != latestVersion) DeleteDownloaderFilesForAllCountries(subdirPath); + // It's OK to remove indexes for absent countries. + DeleteIndexesForAbsentCountries(subdirPath, version, isCountryName); + // Remove subdirectory if it does not contain any files except "." and "..". if (subdir != "." && Platform::IsDirectoryEmpty(subdirPath)) { @@ -146,9 +178,6 @@ void CleanupMapsDirectory(int64_t latestVersion) UNUSED_VALUE(ret); } } - - /// @todo Cleanup temporary index files for already absent mwm files. - /// https://trello.com/c/PKiiOsB4/28-- } void FindAllLocalMapsInDirectory(string const & directory, int64_t version, diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index a283b3e830..e6795ea063 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -3,6 +3,7 @@ #include "platform/country_defines.hpp" #include "platform/local_country_file.hpp" +#include "std/function.hpp" #include "std/shared_ptr.hpp" #include "std/utility.hpp" #include "std/vector.hpp" @@ -17,8 +18,11 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve // Removes partially downloaded maps, empty directories and old // (format v1) maps. Also, removes old (splitted) Japan and Brazil // maps. |version| must be set to the latest data version this app can -// work with. -void CleanupMapsDirectory(int64_t latestVersion); +// work with. |isCountryName| predicate is used to detect and remove +// indexes for absent countries. It must return true for file names +// corresponding to a country. +void CleanupMapsDirectory(int64_t latestVersion, + function const & isCountryName); // Finds all local map files in a directory. Version of these files is // passed as an argument. diff --git a/platform/platform_tests/local_country_file_tests.cpp b/platform/platform_tests/local_country_file_tests.cpp index fa320355b8..b25ab73f20 100644 --- a/platform/platform_tests/local_country_file_tests.cpp +++ b/platform/platform_tests/local_country_file_tests.cpp @@ -137,6 +137,13 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) Platform & platform = GetPlatform(); string const mapsDir = platform.WritableDir(); + // Two fake directories for test country files and indexes. + ScopedDir dir3("3"); + ScopedDir dir4("4"); + + ScopedDir absentCountryIndexesDir(dir4, "Absent"); + ScopedDir irelandIndexesDir(dir4, "Ireland"); + CountryFile japanFile("Japan"); CountryFile brazilFile("Brazil"); CountryFile irelandFile("Ireland"); @@ -147,10 +154,8 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) LocalCountryFile brazilLocalFile(mapsDir, brazilFile, 0 /* version */); ScopedFile brazilMapFile("Brazil.mwm", "Brazil"); - LocalCountryFile irelandLocalFile(mapsDir, irelandFile, 0 /* version */); - ScopedFile irelandMapFile("Ireland.mwm", "Ireland"); - - ScopedDir emptyDir("3"); + LocalCountryFile irelandLocalFile(dir4.GetFullPath(), irelandFile, 4 /* version */); + ScopedFile irelandMapFile(dir4, irelandFile, MapOptions::Map, "Ireland"); // Check that FindAllLocalMaps() vector localFiles; @@ -159,7 +164,10 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) TEST(Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles)); TEST(Contains(localFiles, irelandLocalFile), (irelandLocalFile, localFiles)); - CleanupMapsDirectory(0 /* latestVersion */); + CleanupMapsDirectory(0 /* latestVersion */, [](string const filename) + { + return filename == "Ireland" || filename == "Absent"; + }); japanLocalFile.SyncWithDisk(); TEST_EQUAL(MapOptions::Nothing, japanLocalFile.GetFiles(), ()); @@ -177,8 +185,15 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) TEST(!irelandMapFile.Exists(), (irelandMapFile)); irelandMapFile.Reset(); - TEST(!emptyDir.Exists(), ("Empty directory", emptyDir, "wasn't removed.")); - emptyDir.Reset(); + TEST(!dir3.Exists(), ("Empty directory", dir3, "wasn't removed.")); + dir3.Reset(); + + TEST(dir4.Exists(), ()); + + TEST(!absentCountryIndexesDir.Exists(), ("Indexes for absent country weren't deleted.")); + absentCountryIndexesDir.Reset(); + + TEST(irelandIndexesDir.Exists(), ()); } UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) @@ -200,7 +215,11 @@ UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) {my::JoinFoldersToPath(latestDir.GetRelativePath(), "Russia_Southern.mwm.downloading"), "Southern Russia map"}}; - CleanupMapsDirectory(101010 /* latestVersion */); + auto const isCountryName = [&](string const & /* filename */) + { + return false; + }; + CleanupMapsDirectory(101010 /* latestVersion */, isCountryName); for (ScopedFile & file : toBeDeleted) { diff --git a/platform/platform_tests_support/scoped_dir.cpp b/platform/platform_tests_support/scoped_dir.cpp index 1f27f44e9d..611909b936 100644 --- a/platform/platform_tests_support/scoped_dir.cpp +++ b/platform/platform_tests_support/scoped_dir.cpp @@ -34,6 +34,11 @@ ScopedDir::ScopedDir(string const & relativePath) } } +ScopedDir::ScopedDir(ScopedDir const & parent, string const & name) + : ScopedDir(my::JoinFoldersToPath(parent.GetRelativePath(), name)) +{ +} + ScopedDir::~ScopedDir() { if (m_reset) diff --git a/platform/platform_tests_support/scoped_dir.hpp b/platform/platform_tests_support/scoped_dir.hpp index 8451c0d76a..4095a4777d 100644 --- a/platform/platform_tests_support/scoped_dir.hpp +++ b/platform/platform_tests_support/scoped_dir.hpp @@ -18,6 +18,8 @@ public: /// @param path Path for a testing directory, should be relative to writable-dir. ScopedDir(string const & relativePath); + ScopedDir(ScopedDir const & parent, string const & name); + ~ScopedDir(); inline void Reset() { m_reset = true; } diff --git a/storage/storage.cpp b/storage/storage.cpp index 6c2c331b24..5cadadce8a 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -66,6 +66,12 @@ void DeleteCountryIndexes(LocalCountryFile const & localFile) platform::CountryIndexes::DeleteFromDisk(localFile); } +void DeleteFromDiskWithIndexes(LocalCountryFile const & localFile, MapOptions options) +{ + DeleteCountryIndexes(localFile); + localFile.DeleteFromDisk(options); +} + class EqualFileName { string const & m_name; @@ -130,7 +136,7 @@ void Storage::RegisterAllLocalMaps() LocalCountryFile & localFile = *j; LOG(LINFO, ("Removing obsolete", localFile)); localFile.SyncWithDisk(); - localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); + DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting); ++j; } @@ -340,8 +346,7 @@ void Storage::DeleteCountry(TIndex const & index, MapOptions opt) void Storage::DeleteCustomCountryVersion(LocalCountryFile const & localFile) { CountryFile const countryFile = localFile.GetCountryFile(); - localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); - CountryIndexes::DeleteFromDisk(localFile); + DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting); { auto it = m_localFilesForFakeCountries.find(countryFile); @@ -844,9 +849,8 @@ void Storage::DeleteCountryFiles(TIndex const & index, MapOptions opt) auto & localFiles = it->second; for (auto & localFile : localFiles) { - localFile->DeleteFromDisk(opt); + DeleteFromDiskWithIndexes(*localFile, opt); localFile->SyncWithDisk(); - DeleteCountryIndexes(*localFile); if (localFile->GetFiles() == MapOptions::Nothing) localFile.reset(); } diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index baefeac4d2..304b5e3daa 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -12,6 +12,7 @@ #include "platform/local_country_file.hpp" #include "platform/local_country_file_utils.hpp" #include "platform/platform.hpp" +#include "platform/platform_tests_support/scoped_dir.hpp" #include "platform/platform_tests_support/scoped_file.hpp" #include "coding/file_name_utils.hpp" @@ -732,4 +733,30 @@ UNIT_TEST(StorageTest_EmptyRoutingFile) checker->StartDownload(); runner.Run(); } + +UNIT_TEST(StorageTest_ObsoleteMapsRemoval) +{ + CountryFile country("Azerbaijan"); + + tests_support::ScopedDir dir1("1"); + tests_support::ScopedFile map1(dir1, country, MapOptions::Map, "map1"); + LocalCountryFile file1(dir1.GetFullPath(), country, 1 /* version */); + CountryIndexes::PreparePlaceOnDisk(file1); + + tests_support::ScopedDir dir2("2"); + tests_support::ScopedFile map2(dir2, country, MapOptions::Map, "map2"); + LocalCountryFile file2(dir2.GetFullPath(), country, 2 /* version */); + CountryIndexes::PreparePlaceOnDisk(file2); + + TEST(map1.Exists(), ()); + TEST(map2.Exists(), ()); + + Storage storage; + storage.RegisterAllLocalMaps(); + + TEST(!map1.Exists(), ()); + map1.Reset(); + + TEST(map2.Exists(), ()); +} } // namespace storage From beaaa893aa8af7f50bc8b6e1c344b4015d8b83a8 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Sat, 7 Nov 2015 00:47:47 +0300 Subject: [PATCH 2/3] Implemented single-pass over writable dir. --- generator/generator_tests/check_mwms.cpp | 3 +- map/framework.cpp | 5 - platform/local_country_file.hpp | 3 +- platform/local_country_file_utils.cpp | 223 ++++++++++-------- platform/local_country_file_utils.hpp | 42 ++-- platform/platform.cpp | 5 +- platform/platform.hpp | 5 +- .../local_country_file_tests.cpp | 30 +-- platform/platform_tests/platform_test.cpp | 10 +- .../cross_section_tests.cpp | 2 +- .../routing_test_tools.cpp | 2 +- storage/storage.cpp | 2 +- storage/storage.hpp | 7 +- 13 files changed, 182 insertions(+), 157 deletions(-) diff --git a/generator/generator_tests/check_mwms.cpp b/generator/generator_tests/check_mwms.cpp index 927e308aeb..10fab2cb9e 100644 --- a/generator/generator_tests/check_mwms.cpp +++ b/generator/generator_tests/check_mwms.cpp @@ -16,7 +16,8 @@ UNIT_TEST(CheckMWM_LoadAll) { Platform & platform = GetPlatform(); vector localFiles; - platform::FindAllLocalMapsInDirectory(platform.WritableDir(), 0 /* version */, localFiles); + platform::FindAllLocalMapsInDirectoryAndCleanup(platform.WritableDir(), 0 /* version */, + -1 /* latestVersion */, localFiles); model::FeaturesFetcher m; m.InitClassificator(); diff --git a/map/framework.cpp b/map/framework.cpp index e003c9a7ec..92b052c0fe 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -477,11 +477,6 @@ void Framework::RegisterAllMaps() ("Registering maps while map downloading leads to removing downloading maps from " "ActiveMapsListener::m_items.")); - auto const isCountryName = [this](string const & filename) - { - return m_storage.FindIndexByFile(filename).IsValid(); - }; - platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion(), isCountryName); m_storage.RegisterAllLocalMaps(); int minFormat = numeric_limits::max(); diff --git a/platform/local_country_file.hpp b/platform/local_country_file.hpp index 7d29731f07..7ead5f1d86 100644 --- a/platform/local_country_file.hpp +++ b/platform/local_country_file.hpp @@ -81,7 +81,8 @@ public: private: friend string DebugPrint(LocalCountryFile const &); friend void UnitTest_LocalCountryFile_DirectoryLookup(); - friend void FindAllLocalMaps(vector & localFiles); + friend void FindAllLocalMapsAndCleanup(int64_t latestVersion, + vector & localFiles); /// @note! If directory is empty, the file is stored in resources. /// In this case, the only valid params are m_countryFile and m_version. diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 56525a9e0e..9943522f95 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -77,38 +77,44 @@ string GetSpecialFilesSearchScope() #endif // defined(OMIM_OS_ANDROID) } -void DeleteDownloaderFilesForAllCountries(string const & directory) +class StringsRegexpFilter { - static string const regexp = "\\.(downloading|resume|ready)[0-9]?$"; - Platform::FilesList files; - Platform::GetFilesByRegExp(directory, regexp, files); - for (auto const & file : files) - my::DeleteFileX(my::JoinFoldersToPath(directory, file)); +public: + StringsRegexpFilter(string const & regexp) { regexp::Create(regexp, m_regexp); } + + bool Matches(string const & s) const { return regexp::Matches(s, m_regexp); } + +private: + regexp::RegExpT m_regexp; +}; + +bool IsSpecialName(string const & name) { return name == "." || name == ".."; } + +bool IsDownloaderFile(string const & name) +{ + static StringsRegexpFilter filter(".*\\.(downloading|resume|ready)[0-9]?$"); + return filter.Matches(name); } -void DeleteIndexesForAbsentCountries(string const & directory, - int64_t version, - function const & isCountryName) +bool DirectoryHasIndexesOnly(string const & directory) { - vector files; - FindAllLocalMapsInDirectory(directory, version, files); - - unordered_set names; - for (auto const & file : files) - names.insert(file.GetCountryName()); - - Platform::FilesList subdirs; - Platform::GetFilesByType(directory, Platform::FILE_TYPE_DIRECTORY, subdirs); - for (auto const & subdir : subdirs) + Platform::TFilesWithType fwts; + Platform::GetFilesByType(directory, Platform::FILE_TYPE_REGULAR | Platform::FILE_TYPE_DIRECTORY, + fwts); + for (auto const & fwt : fwts) { - if (subdir == "." || subdir == "..") + auto const & name = fwt.first; + auto const & type = fwt.second; + if (type == Platform::FILE_TYPE_DIRECTORY) + { + if (!IsSpecialName(name)) + return false; continue; - if (!isCountryName(subdir) || names.count(subdir) != 0) - continue; - - LocalCountryFile absentCountry(directory, CountryFile(subdir), version); - CountryIndexes::DeleteFromDisk(absentCountry); + } + if (!CountryIndexes::IsIndexFile(name)) + return false; } + return true; } } // namespace @@ -124,94 +130,93 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve } } -void CleanupMapsDirectory(int64_t latestVersion, - function const & isCountryName) -{ - Platform & platform = GetPlatform(); - - string const mapsDir = platform.WritableDir(); - - { - // Delete Brazil.mwm and Japan.mwm maps, because they was replaces with - // smaler regions after osrm routing implementation. - vector localFiles; - FindAllLocalMapsInDirectory(mapsDir, 0 /* version */, localFiles); - for (LocalCountryFile & localFile : localFiles) - { - string const & countryName = localFile.GetCountryFile().GetNameWithoutExt(); - if (countryName == "Japan" || countryName == "Brazil") - { - localFile.SyncWithDisk(); - localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); - } - } - } - - // Try to delete empty folders. - Platform::FilesList subdirs; - platform.GetFilesByType(mapsDir, Platform::FILE_TYPE_DIRECTORY, subdirs); - for (string const & subdir : subdirs) - { - // No need to visit parent directory. - if (subdir == "..") - continue; - - int64_t version = 0; - if (subdir != "." && !ParseVersion(subdir, version)) - continue; - - string const subdirPath = my::JoinFoldersToPath(mapsDir, subdir); - - // It's OK to remove all temprorary files for maps older than app. - if (version != latestVersion) - DeleteDownloaderFilesForAllCountries(subdirPath); - - // It's OK to remove indexes for absent countries. - DeleteIndexesForAbsentCountries(subdirPath, version, isCountryName); - - // Remove subdirectory if it does not contain any files except "." and "..". - if (subdir != "." && Platform::IsDirectoryEmpty(subdirPath)) - { - Platform::EError const ret = Platform::RmDir(subdirPath); - ASSERT_EQUAL(Platform::ERR_OK, ret, - ("Can't remove empty directory:", subdirPath, "error:", ret)); - UNUSED_VALUE(ret); - } - } -} - -void FindAllLocalMapsInDirectory(string const & directory, int64_t version, - vector & localFiles) +void FindAllLocalMapsInDirectoryAndCleanup(string const & directory, int64_t version, + int64_t latestVersion, + vector & localFiles) { vector files; Platform & platform = GetPlatform(); - platform.GetFilesByRegExp(directory, ".*\\" DATA_FILE_EXTENSION "$", files); - for (string const & file : files) + Platform::TFilesWithType fwts; + platform.GetFilesByType(directory, Platform::FILE_TYPE_REGULAR | Platform::FILE_TYPE_DIRECTORY, + fwts); + + unordered_set names; + for (auto const & fwt : fwts) { + if (fwt.second != Platform::FILE_TYPE_REGULAR) + continue; + + string name = fwt.first; + + // Remove downloader files for old version directories. + if (IsDownloaderFile(name) && version < latestVersion) + { + my::DeleteFileX(my::JoinFoldersToPath(directory, name)); + continue; + } + + if (!strings::EndsWith(name, DATA_FILE_EXTENSION)) + continue; + // Remove DATA_FILE_EXTENSION and use base name as a country file name. - string name = file; my::GetNameWithoutExt(name); - localFiles.emplace_back(directory, CountryFile(name), version); + names.insert(name); + LocalCountryFile localFile(directory, CountryFile(name), version); + + // Delete Brazil.mwm and Japan.mwm maps, because they were + // replaced with smaller regions after osrm routing + // implementation. + if (name == "Japan" || name == "Brazil") + { + localFile.SyncWithDisk(); + localFile.DeleteFromDisk(MapOptions::MapWithCarRouting); + continue; + } + + localFiles.push_back(localFile); + } + + for (auto const & fwt : fwts) + { + if (fwt.second != Platform::FILE_TYPE_DIRECTORY) + continue; + + string name = fwt.first; + if (IsSpecialName(name)) + continue; + + if (names.count(name) == 0 && DirectoryHasIndexesOnly(my::JoinFoldersToPath(directory, name))) + { + // Directory which looks like a directory with indexes for absent country. It's OK to remove + // it. + LocalCountryFile absentCountry(directory, CountryFile(name), version); + CountryIndexes::DeleteFromDisk(absentCountry); + } } } -void FindAllLocalMaps(vector & localFiles) +void FindAllLocalMapsAndCleanup(int64_t latestVersion, vector & localFiles) { - localFiles.clear(); - Platform & platform = GetPlatform(); - string const directory = platform.WritableDir(); - FindAllLocalMapsInDirectory(directory, 0 /* version */, localFiles); + string const dir = platform.WritableDir(); + FindAllLocalMapsInDirectoryAndCleanup(dir, 0 /* version */, latestVersion, localFiles); - Platform::FilesList subdirs; - Platform::GetFilesByType(directory, Platform::FILE_TYPE_DIRECTORY, subdirs); - for (string const & subdir : subdirs) + Platform::TFilesWithType fwts; + Platform::GetFilesByType(dir, Platform::FILE_TYPE_DIRECTORY, fwts); + for (auto const & fwt : fwts) { + string const & subdir = fwt.first; int64_t version; - if (ParseVersion(subdir, version)) - FindAllLocalMapsInDirectory(my::JoinFoldersToPath(directory, subdir), version, localFiles); + if (!ParseVersion(subdir, version)) + continue; + + string const fullPath = my::JoinFoldersToPath(dir, subdir); + FindAllLocalMapsInDirectoryAndCleanup(fullPath, version, latestVersion, localFiles); + Platform::EError err = Platform::RmDir(fullPath); + if (err != Platform::ERR_OK && err != Platform::ERR_DIRECTORY_NOT_EMPTY) + LOG(LWARNING, ("Can't remove directory:", fullPath, err)); } // World and WorldCoasts can be stored in app bundle or in resources @@ -227,10 +232,12 @@ void FindAllLocalMaps(vector & localFiles) try { - ModelReaderPtr reader(platform.GetReader(file + DATA_FILE_EXTENSION, GetSpecialFilesSearchScope())); + ModelReaderPtr reader( + platform.GetReader(file + DATA_FILE_EXTENSION, GetSpecialFilesSearchScope())); // Assume that empty path means the resource file. - LocalCountryFile worldFile(string(), CountryFile(file), version::ReadVersionTimestamp(reader)); + LocalCountryFile worldFile(string(), CountryFile(file), + version::ReadVersionTimestamp(reader)); worldFile.m_files = MapOptions::Map; if (i != localFiles.end()) { @@ -251,6 +258,12 @@ void FindAllLocalMaps(vector & localFiles) } } +void CleanupMapsDirectory(int64_t latestVersion) +{ + vector localFiles; + FindAllLocalMapsAndCleanup(latestVersion, localFiles); +} + bool ParseVersion(string const & s, int64_t & version) { if (s.empty() || s.size() > kMaxTimestampLength) @@ -294,7 +307,10 @@ ModelReader * GetCountryReader(platform::LocalCountryFile const & file, MapOptio Platform & platform = GetPlatform(); // See LocalCountryFile comment for explanation. if (file.GetDirectory().empty()) - return platform.GetReader(file.GetCountryName() + DATA_FILE_EXTENSION, GetSpecialFilesSearchScope()); + { + return platform.GetReader(file.GetCountryName() + DATA_FILE_EXTENSION, + GetSpecialFilesSearchScope()); + } return platform.GetReader(file.GetPath(options), "f"); } @@ -358,6 +374,13 @@ void CountryIndexes::GetIndexesExts(vector & exts) exts.push_back(kOffsetsExt); } +// static +bool CountryIndexes::IsIndexFile(string const & file) +{ + return strings::EndsWith(file, kBitsExt) || strings::EndsWith(file, kNodesExt) || + strings::EndsWith(file, kOffsetsExt); +} + // static string CountryIndexes::IndexesDir(LocalCountryFile const & localFile) { diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index e6795ea063..f241cd3210 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -15,23 +15,19 @@ namespace platform // Removes all files downloader creates during downloading of a country. void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version); -// Removes partially downloaded maps, empty directories and old -// (format v1) maps. Also, removes old (splitted) Japan and Brazil -// maps. |version| must be set to the latest data version this app can -// work with. |isCountryName| predicate is used to detect and remove -// indexes for absent countries. It must return true for file names -// corresponding to a country. -void CleanupMapsDirectory(int64_t latestVersion, +void CleanupMapsDirectory(int64_t latestVersion, vector const & localFiles, function const & isCountryName); -// Finds all local map files in a directory. Version of these files is -// passed as an argument. -void FindAllLocalMapsInDirectory(string const & directory, int64_t version, - vector & localFiles); +// Finds all local map files in |directory|. Version of these files is +// passed as an argument. Also, performs cleanup described in comment +// for FindAllLocalMapsAndCleanup(). +void FindAllLocalMapsInDirectoryAndCleanup(string const & directory, int64_t version, + int64_t latestVersion, + vector & localFiles); -// Finds all local map files in resources and writable -// directory. Also, for Android, checks /Android/obb directory. -// Directories should have the following structure: +// Finds all local map files in resources and writable directory. For +// Android, checks /Android/obb directory. Subdirectories in the +// writable directory should have the following structure: // // dir/*.mwm -- map files, base name should correspond to countries.txt, // -- version is assumed to be zero (unknown). @@ -42,10 +38,17 @@ void FindAllLocalMapsInDirectory(string const & directory, int64_t version, // dir/[0-9]{1,18}/*.mwm.routing -- routing file for corresponding map files, // -- version is assumed to be the name of a directory. // -// We can't derive version of a map file from its header because -// currently header stores format version + individual mwm's file -// generation timestamp, not timestamp mentioned in countries.txt. -void FindAllLocalMaps(vector & localFiles); +// Also, this method performs cleanup described in a comment for +// CleanupMapsDirectory(). +void FindAllLocalMapsAndCleanup(int64_t latestVersion, vector & localFiles); + +// This method removes: +// * partially downloaded non-latest maps (with version less than |latestVersion|) +// * empty directories +// * old (format v1) maps +// * old (splitted) Japan and Brazil maps +// * indexes for absent countries +void CleanupMapsDirectory(int64_t latestVersion); // Tries to parse a version from a string of size not longer than 18 // symbols and representing an unsigned decimal number. Leading zeroes @@ -88,6 +91,9 @@ public: // Pushes to the exts's end possible index files extensions. static void GetIndexesExts(vector & exts); + // Returns true if |file| corresponds to an index file. + static bool IsIndexFile(string const & file); + private: friend void UnitTest_LocalCountryFile_CountryIndexes(); friend void UnitTest_LocalCountryFile_DoNotDeleteUserFiles(); diff --git a/platform/platform.cpp b/platform/platform.cpp index c34b28a1b9..fc9cc430d7 100644 --- a/platform/platform.cpp +++ b/platform/platform.cpp @@ -120,7 +120,8 @@ void Platform::GetFilesByExt(string const & directory, string const & ext, Files } // static -void Platform::GetFilesByType(string const & directory, unsigned typeMask, FilesList & outFiles) +void Platform::GetFilesByType(string const & directory, unsigned typeMask, + TFilesWithType & outFiles) { FilesList allFiles; GetFilesByRegExp(directory, ".*", allFiles); @@ -130,7 +131,7 @@ void Platform::GetFilesByType(string const & directory, unsigned typeMask, Files if (GetFileType(my::JoinFoldersToPath(directory, file), type) != ERR_OK) continue; if (typeMask & type) - outFiles.push_back(file); + outFiles.emplace_back(file, type); } } diff --git a/platform/platform.hpp b/platform/platform.hpp index 312196ac3b..d7e071dc38 100644 --- a/platform/platform.hpp +++ b/platform/platform.hpp @@ -49,6 +49,8 @@ public: CONNECTION_WWAN }; + using TFilesWithType = vector>; + protected: /// Usually read-only directory for application resources string m_resourcesDir; @@ -138,7 +140,8 @@ public: static void GetFilesByRegExp(string const & directory, string const & regexp, FilesList & outFiles); //@} - static void GetFilesByType(string const & directory, unsigned typeMask, FilesList & outFiles); + static void GetFilesByType(string const & directory, unsigned typeMask, + TFilesWithType & outFiles); static bool IsDirectoryEmpty(string const & directory); diff --git a/platform/platform_tests/local_country_file_tests.cpp b/platform/platform_tests/local_country_file_tests.cpp index b25ab73f20..87527a990f 100644 --- a/platform/platform_tests/local_country_file_tests.cpp +++ b/platform/platform_tests/local_country_file_tests.cpp @@ -157,25 +157,16 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles) LocalCountryFile irelandLocalFile(dir4.GetFullPath(), irelandFile, 4 /* version */); ScopedFile irelandMapFile(dir4, irelandFile, MapOptions::Map, "Ireland"); - // Check that FindAllLocalMaps() + // Check FindAllLocalMaps() vector localFiles; - FindAllLocalMaps(localFiles); - TEST(Contains(localFiles, japanLocalFile), (japanLocalFile, localFiles)); - TEST(Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles)); + FindAllLocalMapsAndCleanup(-1 /* latestVersion */, localFiles); + TEST(!Contains(localFiles, japanLocalFile), (japanLocalFile, localFiles)); + TEST(!Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles)); TEST(Contains(localFiles, irelandLocalFile), (irelandLocalFile, localFiles)); - CleanupMapsDirectory(0 /* latestVersion */, [](string const filename) - { - return filename == "Ireland" || filename == "Absent"; - }); - - japanLocalFile.SyncWithDisk(); - TEST_EQUAL(MapOptions::Nothing, japanLocalFile.GetFiles(), ()); TEST(!japanMapFile.Exists(), (japanMapFile)); japanMapFile.Reset(); - brazilLocalFile.SyncWithDisk(); - TEST_EQUAL(MapOptions::Nothing, brazilLocalFile.GetFiles(), ()); TEST(!brazilMapFile.Exists(), (brazilMapFile)); brazilMapFile.Reset(); @@ -215,11 +206,7 @@ UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles) {my::JoinFoldersToPath(latestDir.GetRelativePath(), "Russia_Southern.mwm.downloading"), "Southern Russia map"}}; - auto const isCountryName = [&](string const & /* filename */) - { - return false; - }; - CleanupMapsDirectory(101010 /* latestVersion */, isCountryName); + CleanupMapsDirectory(101010 /* latestVersion */); for (ScopedFile & file : toBeDeleted) { @@ -254,7 +241,8 @@ UNIT_TEST(LocalCountryFile_DirectoryLookup) "Netherlands-routing"); vector localFiles; - FindAllLocalMapsInDirectory(testDir.GetFullPath(), 150309, localFiles); + FindAllLocalMapsInDirectoryAndCleanup(testDir.GetFullPath(), 150309 /* version */, + -1 /* latestVersion */, localFiles); sort(localFiles.begin(), localFiles.end()); for (LocalCountryFile & localFile : localFiles) localFile.SyncWithDisk(); @@ -278,11 +266,11 @@ UNIT_TEST(LocalCountryFile_AllLocalFilesLookup) { CountryFile const italyFile("Italy"); - ScopedDir testDir("010101"); + ScopedDir testDir("10101"); ScopedFile testItalyMapFile(testDir, italyFile, MapOptions::Map, "Italy-map"); vector localFiles; - FindAllLocalMaps(localFiles); + FindAllLocalMapsAndCleanup(-1 /* latestVersion */, localFiles); multiset localFilesSet(localFiles.begin(), localFiles.end()); bool worldFound = false; diff --git a/platform/platform_tests/platform_test.cpp b/platform/platform_tests/platform_test.cpp index 851b48cf9c..14789475e6 100644 --- a/platform/platform_tests/platform_test.cpp +++ b/platform/platform_tests/platform_test.cpp @@ -22,9 +22,13 @@ char const * TEST_FILE_NAME = "some_temporary_unit_test_file.tmp"; void CheckFilesPresence(string const & baseDir, unsigned typeMask, initializer_list> const & files) { - Platform::FilesList filesList; - Platform::GetFilesByType(baseDir, typeMask, filesList); - multiset filesSet(filesList.begin(), filesList.end()); + Platform::TFilesWithType fwts; + Platform::GetFilesByType(baseDir, typeMask, fwts); + + multiset filesSet; + for (auto const & fwt : fwts) + filesSet.insert(fwt.first); + for (auto const & file : files) TEST_EQUAL(filesSet.count(file.first), file.second, (file.first, file.second)); } diff --git a/routing/routing_integration_tests/cross_section_tests.cpp b/routing/routing_integration_tests/cross_section_tests.cpp index 6a5b89dda0..50f17023ba 100644 --- a/routing/routing_integration_tests/cross_section_tests.cpp +++ b/routing/routing_integration_tests/cross_section_tests.cpp @@ -18,7 +18,7 @@ UNIT_TEST(CheckCrossSections) { static double constexpr kPointEquality = 0.01; vector localFiles; - platform::FindAllLocalMaps(localFiles); + platform::FindAllLocalMapsAndCleanup(-1 /* latestVersion */, localFiles); size_t ingoingErrors = 0; size_t outgoingErrors = 0; diff --git a/routing/routing_integration_tests/routing_test_tools.cpp b/routing/routing_integration_tests/routing_test_tools.cpp index d0a6d6f3cb..e90a9ada13 100644 --- a/routing/routing_integration_tests/routing_test_tools.cpp +++ b/routing/routing_integration_tests/routing_test_tools.cpp @@ -159,7 +159,7 @@ namespace integration pl.SetResourceDir(options.m_resourcePath); vector localFiles; - platform::FindAllLocalMaps(localFiles); + platform::FindAllLocalMapsAndCleanup(-1, localFiles); for (auto & file : localFiles) file.SyncWithDisk(); ASSERT(!localFiles.empty(), ()); diff --git a/storage/storage.cpp b/storage/storage.cpp index 5cadadce8a..f7d9794f49 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -111,7 +111,7 @@ void Storage::RegisterAllLocalMaps() m_localFilesForFakeCountries.clear(); vector localFiles; - FindAllLocalMaps(localFiles); + FindAllLocalMapsAndCleanup(GetCurrentDataVersion(), localFiles); auto compareByCountryAndVersion = [](LocalCountryFile const & lhs, LocalCountryFile const & rhs) { diff --git a/storage/storage.hpp b/storage/storage.hpp index 8016d375df..95a3baac62 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -6,6 +6,8 @@ #include "storage/queued_country.hpp" #include "storage/storage_defines.hpp" +#include "platform/local_country_file.hpp" + #include "std/function.hpp" #include "std/list.hpp" #include "std/set.hpp" @@ -14,7 +16,6 @@ #include "std/unique_ptr.hpp" #include "std/vector.hpp" - namespace storage { /// Can be used to store local maps and/or maps available for download @@ -48,7 +49,9 @@ private: using TLocalFilePtr = shared_ptr; map> m_localFiles; - // Our World.mwm and WorldCoasts.mwm are fake countries, together with any custom mwm in data folder. + + // Our World.mwm and WorldCoasts.mwm are fake countries, together with any custom mwm in data + // folder. map m_localFilesForFakeCountries; /// used to correctly calculate total country download progress with more than 1 file From fe979b831c066a26ba590bf81bb204b1220a6d84 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Mon, 9 Nov 2015 16:17:59 +0300 Subject: [PATCH 3/3] Review fixes. --- platform/local_country_file_utils.cpp | 2 +- platform/local_country_file_utils.hpp | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 9943522f95..85c8613a7f 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -92,7 +92,7 @@ bool IsSpecialName(string const & name) { return name == "." || name == ".."; } bool IsDownloaderFile(string const & name) { - static StringsRegexpFilter filter(".*\\.(downloading|resume|ready)[0-9]?$"); + static StringsRegexpFilter const filter(".*\\.(downloading|resume|ready)[0-9]?$"); return filter.Matches(name); } diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index f241cd3210..cee7f88e69 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -15,9 +15,6 @@ namespace platform // Removes all files downloader creates during downloading of a country. void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version); -void CleanupMapsDirectory(int64_t latestVersion, vector const & localFiles, - function const & isCountryName); - // Finds all local map files in |directory|. Version of these files is // passed as an argument. Also, performs cleanup described in comment // for FindAllLocalMapsAndCleanup(). @@ -46,7 +43,7 @@ void FindAllLocalMapsAndCleanup(int64_t latestVersion, vector // * partially downloaded non-latest maps (with version less than |latestVersion|) // * empty directories // * old (format v1) maps -// * old (splitted) Japan and Brazil maps +// * old (split) Japan and Brazil maps // * indexes for absent countries void CleanupMapsDirectory(int64_t latestVersion);