From 48aefc1983388631ea2164a0111692eae379ce9a Mon Sep 17 00:00:00 2001 From: allexzander Date: Wed, 16 Jun 2021 21:56:37 +0300 Subject: [PATCH 1/5] Fix VFS crash and false conflict on local new. Signed-off-by: allexzander --- src/libsync/discovery.cpp | 76 ++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index da6f48bff1454..4ce8e1af6f73f 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -897,44 +897,54 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( // TODO: We may want to execute the same logic for non-VFS mode, as, moving/renaming the same folder by 2 or more clients at the same time is not possible in Web UI. // Keeping it like this (for VFS files and folders only) just to fix a user issue. const auto isVfsEnabled = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() != Vfs::Off; - if (localEntry.isVirtualFile || (localEntry.isDirectory && isVfsEnabled)) { - // must be a dehydrated placeholder - const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local); - // a folder must be online-only (no files should be hydrated) - const bool isFolderPlaceholder = localEntry.isDirectory && *_discoveryData->_syncOptions._vfs->availability(path._local) == VfsItemAvailability::OnlineOnly; + // must be a dehydrated placeholder + const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local); - Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW); - if (item->_instruction != CSYNC_INSTRUCTION_NEW) { - qCWarning(lcDisco) << "Wiping virtual file without db entry for" << path._local << ". But, item->_instruction is" << item->_instruction; - } + // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet + const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityError::NoSuchItem; - // must be a file placeholder or an online-only folder placeholder - if (isFilePlaceHolder || isFolderPlaceholder) { - if (isFolderPlaceholder) { - qCInfo(lcDisco) << "Wiping virtual folder without db entry for" << path._local; - } else { - qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local; - } - item->_instruction = CSYNC_INSTRUCTION_REMOVE; - item->_direction = SyncFileItem::Down; - // this flag needs to be unset, otherwise a folder would get marked as new in the processSubJobs - _childModified = false; - if (isFolderPlaceholder && _discoveryData) { - emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local); - } - } else { - if (localEntry.isDirectory && !isFolderPlaceholder) { - qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload."; - if (_discoveryData) { - emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local); - } - return; + if (!isFilePlaceHolder && !folderAvailability) { + // not a file placeholder and not a synced folder placeholder (new local folder) + return; + } + + // a folder must be online-only (no files should be hydrated) + const auto isOnlineOnlyFolder = folderAvailability && *folderAvailability == VfsItemAvailability::OnlineOnly; + + if (!isFilePlaceHolder && !isOnlineOnlyFolder) { + // either a VFS file without db entry or a folder with one or more hydrated files + if (localEntry.isDirectory && folderAvailability && *folderAvailability != VfsItemAvailability::OnlineOnly) { + qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload."; + if (_discoveryData) { + emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local); } - qCWarning(lcDisco) << "Virtual file without db entry for" << path._local - << "but looks odd, keeping"; - item->_instruction = CSYNC_INSTRUCTION_IGNORE; + return; } + qCWarning(lcDisco) << "Virtual file without db entry for" << path._local + << "but looks odd, keeping"; + item->_instruction = CSYNC_INSTRUCTION_IGNORE; + + return; + } + + Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW); + if (item->_instruction != CSYNC_INSTRUCTION_NEW) { + qCWarning(lcDisco) << "Wiping virtual file without db entry for" << path._local << ". But, item->_instruction is" << item->_instruction; + return; + } + + if (isOnlineOnlyFolder) { + qCInfo(lcDisco) << "Wiping virtual folder without db entry for" << path._local; + } else { + qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local; + } + item->_instruction = CSYNC_INSTRUCTION_REMOVE; + item->_direction = SyncFileItem::Down; + // this flag needs to be unset, otherwise a folder would get marked as new in the processSubJobs + _childModified = false; + if (isOnlineOnlyFolder && _discoveryData) { + emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local); } }; From c5982143aa3c7a1c38d62fd54113793f572b2505 Mon Sep 17 00:00:00 2001 From: allexzander Date: Thu, 17 Jun 2021 10:22:44 +0300 Subject: [PATCH 2/5] Refactoring. Signed-off-by: allexzander --- src/libsync/discovery.cpp | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 4ce8e1af6f73f..5e3bcfb2ea045 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -896,7 +896,17 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( auto postProcessLocalNew = [item, localEntry, path, this]() { // TODO: We may want to execute the same logic for non-VFS mode, as, moving/renaming the same folder by 2 or more clients at the same time is not possible in Web UI. // Keeping it like this (for VFS files and folders only) just to fix a user issue. - const auto isVfsEnabled = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() != Vfs::Off; + + if (!(_discoveryData && _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() != Vfs::Off)) { + // for VFS files and folders only + return; + } + + Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW); + if (item->_instruction != CSYNC_INSTRUCTION_NEW) { + qCWarning(lcDisco) << "Trying to wipe a virtual item" << path._local << " with item->_instruction" << item->_instruction; + return; + } // must be a dehydrated placeholder const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local); @@ -913,12 +923,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( const auto isOnlineOnlyFolder = folderAvailability && *folderAvailability == VfsItemAvailability::OnlineOnly; if (!isFilePlaceHolder && !isOnlineOnlyFolder) { - // either a VFS file without db entry or a folder with one or more hydrated files - if (localEntry.isDirectory && folderAvailability && *folderAvailability != VfsItemAvailability::OnlineOnly) { + if (localEntry.isDirectory && folderAvailability && !isOnlineOnlyFolder) { + // a VFS folder but is not online0only (has some files hydrated) qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload."; - if (_discoveryData) { - emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local); - } + emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local); return; } qCWarning(lcDisco) << "Virtual file without db entry for" << path._local @@ -928,14 +936,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( return; } - Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW); - if (item->_instruction != CSYNC_INSTRUCTION_NEW) { - qCWarning(lcDisco) << "Wiping virtual file without db entry for" << path._local << ". But, item->_instruction is" << item->_instruction; - return; - } - if (isOnlineOnlyFolder) { + // if we're wiping a folder, we will only get this function called once and will wipe a folder along with it's files and also display one error in GUI qCInfo(lcDisco) << "Wiping virtual folder without db entry for" << path._local; + emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local); } else { qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local; } @@ -943,9 +947,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( item->_direction = SyncFileItem::Down; // this flag needs to be unset, otherwise a folder would get marked as new in the processSubJobs _childModified = false; - if (isOnlineOnlyFolder && _discoveryData) { - emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local); - } }; // Check if it is a move From 22eba41cb85b9dcbee2d075b7bce5cf11411eda7 Mon Sep 17 00:00:00 2001 From: allexzander Date: Thu, 17 Jun 2021 13:09:01 +0300 Subject: [PATCH 3/5] Do not process non-directories and non-virtual files. Wipe empty online-only folder based on its pinState. Signed-off-by: allexzander --- src/libsync/discovery.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 5e3bcfb2ea045..d4dc0689fedd2 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -902,6 +902,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( return; } + if (!localEntry.isVirtualFile && !localEntry.isDirectory) { + return; + } + Q_ASSERT(item->_instruction == CSYNC_INSTRUCTION_NEW); if (item->_instruction != CSYNC_INSTRUCTION_NEW) { qCWarning(lcDisco) << "Trying to wipe a virtual item" << path._local << " with item->_instruction" << item->_instruction; @@ -914,13 +918,19 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityError::NoSuchItem; - if (!isFilePlaceHolder && !folderAvailability) { + const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : PinState::Unspecified; + + if (!isFilePlaceHolder && !folderAvailability && !folderPinState) { // not a file placeholder and not a synced folder placeholder (new local folder) return; } - // a folder must be online-only (no files should be hydrated) - const auto isOnlineOnlyFolder = folderAvailability && *folderAvailability == VfsItemAvailability::OnlineOnly; + const auto isFolderPinStateOnlineOnly = (folderPinState && *folderPinState == PinState::OnlineOnly); + + const auto isFolderAvailabilityOnlineOnly = (folderAvailability && *folderAvailability == VfsItemAvailability::OnlineOnly); + + // a folder is considered online-only if: no files are hydrated, or, if it's an empty folder + const auto isOnlineOnlyFolder = isFolderAvailabilityOnlineOnly || !folderAvailability && isFolderPinStateOnlineOnly; if (!isFilePlaceHolder && !isOnlineOnlyFolder) { if (localEntry.isDirectory && folderAvailability && !isOnlineOnlyFolder) { @@ -942,6 +952,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a folder. It's going to get cleared!"), path._local); } else { qCInfo(lcDisco) << "Wiping virtual file without db entry for" << path._local; + emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading a file. It's going to get removed!"), path._local); } item->_instruction = CSYNC_INSTRUCTION_REMOVE; item->_direction = SyncFileItem::Down; From 6ce5279b6c3bc78bb06b98bcb7a310660fbc89c3 Mon Sep 17 00:00:00 2001 From: allexzander Date: Fri, 18 Jun 2021 15:18:12 +0300 Subject: [PATCH 4/5] Fix review comments. Signed-off-by: allexzander --- src/common/result.h | 2 ++ src/libsync/discovery.cpp | 12 ++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/common/result.h b/src/common/result.h index 579914f5c5537..25129e8751677 100644 --- a/src/common/result.h +++ b/src/common/result.h @@ -126,6 +126,8 @@ class Result ASSERT(_isError); return std::move(_error); } + + bool isValid() const { return !_isError; } }; namespace detail { diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index d4dc0689fedd2..777b2390e6f95 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -916,24 +916,24 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local); // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet - const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityError::NoSuchItem; + const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityResult(Vfs::AvailabilityError::NoSuchItem); - const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : PinState::Unspecified; + const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : Optional(PinState::Unspecified); - if (!isFilePlaceHolder && !folderAvailability && !folderPinState) { + if (!isFilePlaceHolder && !folderAvailability.isValid() && !folderPinState.isValid()) { // not a file placeholder and not a synced folder placeholder (new local folder) return; } - const auto isFolderPinStateOnlineOnly = (folderPinState && *folderPinState == PinState::OnlineOnly); + const auto isFolderPinStateOnlineOnly = (folderPinState.isValid() && *folderPinState == PinState::OnlineOnly); - const auto isFolderAvailabilityOnlineOnly = (folderAvailability && *folderAvailability == VfsItemAvailability::OnlineOnly); + const auto isFolderAvailabilityOnlineOnly = (folderAvailability.isValid() && *folderAvailability == VfsItemAvailability::OnlineOnly); // a folder is considered online-only if: no files are hydrated, or, if it's an empty folder const auto isOnlineOnlyFolder = isFolderAvailabilityOnlineOnly || !folderAvailability && isFolderPinStateOnlineOnly; if (!isFilePlaceHolder && !isOnlineOnlyFolder) { - if (localEntry.isDirectory && folderAvailability && !isOnlineOnlyFolder) { + if (localEntry.isDirectory && folderAvailability.isValid() && !isOnlineOnlyFolder) { // a VFS folder but is not online0only (has some files hydrated) qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload."; emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local); From 99f6e822906ba47670593dc91ee1a601ee0fcdcc Mon Sep 17 00:00:00 2001 From: allexzander Date: Thu, 1 Jul 2021 11:58:23 +0300 Subject: [PATCH 5/5] Fix review comments. Signed-off-by: allexzander --- src/libsync/discovery.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 777b2390e6f95..240bbc0baba02 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -916,24 +916,24 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( const bool isFilePlaceHolder = !localEntry.isDirectory && _discoveryData->_syncOptions._vfs->isDehydratedPlaceholder(_discoveryData->_localDir + path._local); // either correct availability, or a result with error if the folder is new or otherwise has no availability set yet - const auto folderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityResult(Vfs::AvailabilityError::NoSuchItem); + const auto folderPlaceHolderAvailability = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->availability(path._local) : Vfs::AvailabilityResult(Vfs::AvailabilityError::NoSuchItem); const auto folderPinState = localEntry.isDirectory ? _discoveryData->_syncOptions._vfs->pinState(path._local) : Optional(PinState::Unspecified); - if (!isFilePlaceHolder && !folderAvailability.isValid() && !folderPinState.isValid()) { + if (!isFilePlaceHolder && !folderPlaceHolderAvailability.isValid() && !folderPinState.isValid()) { // not a file placeholder and not a synced folder placeholder (new local folder) return; } const auto isFolderPinStateOnlineOnly = (folderPinState.isValid() && *folderPinState == PinState::OnlineOnly); - const auto isFolderAvailabilityOnlineOnly = (folderAvailability.isValid() && *folderAvailability == VfsItemAvailability::OnlineOnly); + const auto isfolderPlaceHolderAvailabilityOnlineOnly = (folderPlaceHolderAvailability.isValid() && *folderPlaceHolderAvailability == VfsItemAvailability::OnlineOnly); // a folder is considered online-only if: no files are hydrated, or, if it's an empty folder - const auto isOnlineOnlyFolder = isFolderAvailabilityOnlineOnly || !folderAvailability && isFolderPinStateOnlineOnly; + const auto isOnlineOnlyFolder = isfolderPlaceHolderAvailabilityOnlineOnly || !folderPlaceHolderAvailability && isFolderPinStateOnlineOnly; if (!isFilePlaceHolder && !isOnlineOnlyFolder) { - if (localEntry.isDirectory && folderAvailability.isValid() && !isOnlineOnlyFolder) { + if (localEntry.isDirectory && folderPlaceHolderAvailability.isValid() && !isOnlineOnlyFolder) { // a VFS folder but is not online0only (has some files hydrated) qCInfo(lcDisco) << "Virtual directory without db entry for" << path._local << "but it contains hydrated file(s), so let's keep it and reupload."; emit _discoveryData->addErrorToGui(SyncFileItem::SoftError, tr("Conflict when uploading some files to a folder. Those, conflicted, are going to get cleared!"), path._local);