diff --git a/.clang-tidy b/.clang-tidy index 25df52b1a..21c4d97cd 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,5 +1,6 @@ Checks: '-*, bugprone-argument-comment, + bugprone-branch-clone, cppcoreguidelines-init-variables, misc-*, -misc-non-private-member-variables-in-classes, diff --git a/src/csync/csync_reconcile.cpp b/src/csync/csync_reconcile.cpp index af5fbaa41..0681ce4ae 100644 --- a/src/csync/csync_reconcile.cpp +++ b/src/csync/csync_reconcile.cpp @@ -188,11 +188,12 @@ static void _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) }(); auto curParent = our_tree->findFile(curParentPath); - if(!other) { - // Stick with the NEW - return; - } else if (!other->e2eMangledName.isEmpty() || (curParent && curParent->isE2eEncrypted)) { - // Stick with the NEW as well, we want to always issue delete + upload in such cases + if (!other + || !other->e2eMangledName.isEmpty() + || (curParent && curParent->isE2eEncrypted)) { + // Stick with the NEW since there's no "other" file + // or if there's an "other" file it involves E2EE and + // we want to always issue delete + upload in such cases return; } else if (other->instruction == CSYNC_INSTRUCTION_RENAME) { // Some other EVAL_RENAME already claimed other. diff --git a/src/gui/folderstatusmodel.cpp b/src/gui/folderstatusmodel.cpp index af0a3b998..92e6b93e7 100644 --- a/src/gui/folderstatusmodel.cpp +++ b/src/gui/folderstatusmodel.cpp @@ -250,9 +250,7 @@ QVariant FolderStatusModel::data(const QModelIndex &index, int role) const if (f->syncPaused()) { return theme->folderDisabledIcon(); } else { - if (status == SyncResult::SyncPrepare) { - return theme->syncStateIcon(SyncResult::SyncRunning); - } else if (status == SyncResult::Undefined) { + if (status == SyncResult::SyncPrepare || status == SyncResult::Undefined) { return theme->syncStateIcon(SyncResult::SyncRunning); } else { // The "Problem" *result* just means some files weren't @@ -1072,7 +1070,7 @@ void FolderStatusModel::slotFolderSyncStateChange(Folder *f) auto &pi = _folders[folderIndex]._progress; SyncResult::Status state = f->syncResult().status(); - if (!f->canSync()) { + if (!f->canSync() || state == SyncResult::Problem || state == SyncResult::Success || state == SyncResult::Error) { // Reset progress info. pi = SubFolderInfo::Progress(); } else if (state == SyncResult::NotYetStarted) { @@ -1093,11 +1091,6 @@ void FolderStatusModel::slotFolderSyncStateChange(Folder *f) } else if (state == SyncResult::SyncPrepare) { pi = SubFolderInfo::Progress(); pi._overallSyncString = tr("Preparing to sync …"); - } else if (state == SyncResult::Problem || state == SyncResult::Success) { - // Reset the progress info after a sync. - pi = SubFolderInfo::Progress(); - } else if (state == SyncResult::Error) { - pi = SubFolderInfo::Progress(); } // update the icon etc. now diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp index 021e4e02f..16eed86f6 100644 --- a/src/gui/owncloudgui.cpp +++ b/src/gui/owncloudgui.cpp @@ -413,14 +413,17 @@ void ownCloudGui::slotUpdateProgress(const QString &folder, const ProgressInfo & { Q_UNUSED(folder); + // FIXME: Lots of messages computed for nothing in this method, needs revisiting if (progress.status() == ProgressInfo::Discovery) { +#if 0 if (!progress._currentDiscoveredRemoteFolder.isEmpty()) { - //_actionStatus->setText(tr("Checking for changes in remote '%1'") - //.arg(progress._currentDiscoveredRemoteFolder)); + _actionStatus->setText(tr("Checking for changes in remote '%1'") + .arg(progress._currentDiscoveredRemoteFolder)); } else if (!progress._currentDiscoveredLocalFolder.isEmpty()) { - //_actionStatus->setText(tr("Checking for changes in local '%1'") - //.arg(progress._currentDiscoveredLocalFolder)); + _actionStatus->setText(tr("Checking for changes in local '%1'") + .arg(progress._currentDiscoveredLocalFolder)); } +#endif } else if (progress.status() == ProgressInfo::Done) { QTimer::singleShot(2000, this, &ownCloudGui::slotComputeOverallSyncStatus); } diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 3c25fe38a..112f1caf8 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -159,12 +159,11 @@ void DiscoveryJob::update_job_update_callback(bool local, auto *updateJob = static_cast(userdata); if (updateJob) { // Don't wanna overload the UI - if (!updateJob->_lastUpdateProgressCallbackCall.isValid()) { - updateJob->_lastUpdateProgressCallbackCall.start(); // first call - } else if (updateJob->_lastUpdateProgressCallbackCall.elapsed() < 200) { - return; - } else { + if (!updateJob->_lastUpdateProgressCallbackCall.isValid() + || updateJob->_lastUpdateProgressCallbackCall.elapsed() >= 200) { updateJob->_lastUpdateProgressCallbackCall.start(); + } else { + return; } QByteArray pPath(dirUrl); diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp index d40ccede4..fc8464558 100644 --- a/src/libsync/networkjobs.cpp +++ b/src/libsync/networkjobs.cpp @@ -385,11 +385,8 @@ bool LsColJob::finished() // XML parse error emit finishedWithError(reply()); } - } else if (httpCode == 207) { - // wrong content type - emit finishedWithError(reply()); } else { - // wrong HTTP code or any other network error + // wrong content type, wrong HTTP code or any other network error emit finishedWithError(reply()); } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 88f32f6e8..ef41bd004 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -404,24 +404,21 @@ void OwncloudPropagator::start(const SyncFileItemVector &items, // this is an item in a directory which is going to be removed. auto *delDirJob = qobject_cast(directoriesToRemove.first()); - if (item->_instruction == CSYNC_INSTRUCTION_REMOVE) { - // already taken care of. (by the removal of the parent directory) + const auto isNewDirectory = item->isDirectory() && + (item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_TYPE_CHANGE); + + if (item->_instruction == CSYNC_INSTRUCTION_REMOVE || isNewDirectory) { + // If it is a remove it is already taken care of by the removal of the parent directory + + // If it is a new directory then it is inside a deleted directory... That can happen if + // the directory etag was not fetched properly on the previous sync because the sync was + // aborted while uploading this directory (which is now removed). We can ignore it. // increase the number of subjobs that would be there. if (delDirJob) { delDirJob->increaseAffectedCount(); } continue; - } else if (item->isDirectory() - && (item->_instruction == CSYNC_INSTRUCTION_NEW - || item->_instruction == CSYNC_INSTRUCTION_TYPE_CHANGE)) { - // create a new directory within a deleted directory? That can happen if the directory - // etag was not fetched properly on the previous sync because the sync was aborted - // while uploading this directory (which is now removed). We can ignore it. - if (delDirJob) { - delDirJob->increaseAffectedCount(); - } - continue; } else if (item->_instruction == CSYNC_INSTRUCTION_IGNORE) { continue; } else if (item->_instruction == CSYNC_INSTRUCTION_RENAME) { diff --git a/src/libsync/progressdispatcher.cpp b/src/libsync/progressdispatcher.cpp index 518bd9ec0..4b571609f 100644 --- a/src/libsync/progressdispatcher.cpp +++ b/src/libsync/progressdispatcher.cpp @@ -74,7 +74,6 @@ QString Progress::asActionString(const SyncFileItem &item) case CSYNC_INSTRUCTION_IGNORE: return QCoreApplication::translate("progress", "ignoring"); case CSYNC_INSTRUCTION_STAT_ERROR: - return QCoreApplication::translate("progress", "error"); case CSYNC_INSTRUCTION_ERROR: return QCoreApplication::translate("progress", "error"); case CSYNC_INSTRUCTION_UPDATE_METADATA: diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 152d66b29..d479c15b6 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -1567,17 +1567,17 @@ void SyncEngine::checkForPermission(SyncFileItemVector &syncItems) const auto filePerms = getPermissions((*it)->_file); //true when it is just a rename in the same directory. (not a move) - bool isRename = (*it)->_file.startsWith(parentDir) && (*it)->_file.lastIndexOf('/') == slashPos; + const bool isRename = (*it)->_file.startsWith(parentDir) && (*it)->_file.lastIndexOf('/') == slashPos; + const bool isForbiddenSubDirectoryCreation = (*it)->isDirectory() && !destPerms.hasPermission(RemotePermissions::CanAddSubDirectories); + const bool isForbiddenFileCreation = !(*it)->isDirectory() && !destPerms.hasPermission(RemotePermissions::CanAddFile); // Check if we are allowed to move to the destination. bool destinationOK = true; if (isRename || destPerms.isNull()) { // no need to check for the destination dir permission destinationOK = true; - } else if ((*it)->isDirectory() && !destPerms.hasPermission(RemotePermissions::CanAddSubDirectories)) { - destinationOK = false; - } else if (!(*it)->isDirectory() && !destPerms.hasPermission(RemotePermissions::CanAddFile)) { + } else if (isForbiddenSubDirectoryCreation || isForbiddenFileCreation) { destinationOK = false; } @@ -1694,7 +1694,6 @@ void SyncEngine::restoreOldFiles(SyncFileItemVector &syncItems) case CSYNC_INSTRUCTION_NEW: // Ideally we should try to revert the rename or remove, but this would be dangerous // without re-doing the reconcile phase. So just let it happen. - break; default: break; }