Enable the bugprone-branch-clone clang-tidy check

Signed-off-by: Kevin Ottens <kevin.ottens@nextcloud.com>

diff --git a/.clang-tidy b/.clang-tidy
index 25df52b1ae29bda2e3d307c191d93eb943f56f6f..21c4d97cde0f03baf1713a04c7ea81df9071bc52 100644 (file)
--- 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 af5fbaa4108543ba1def838024afd4dbabaa14cc..0681ce4ae794b6f13bb26f1bdc6ebc7cad930992 100644 (file)
--- 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 af0a3b9983537063acbb4b98c67f5f96c9694ae7..92e6b93e7bc86311eef5949a327b49a3157f15d0 100644 (file)
--- 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 021e4e02ffa11c3648e31d7168fff1086898cd00..16eed86f6a0a126aea0e4295980102dfc179911f 100644 (file)
--- 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 3c25fe38ac1270f283dfc1371139eae4d14a0091..112f1caf8c1b73f64092c0dac06384c54d693bdd 100644 (file)
--- 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<DiscoveryJob *>(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 d40ccede4764c523cf852d757f0b36f8fd2c84d8..fc8464558d3646697b8f8a2f5fb40ddf55adf17a 100644 (file)
--- 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 88f32f6e8ede2054194703e11575df6948170fce..ef41bd00481b94b4d82864aabb9b93f6039aa9f0 100644 (file)
--- 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<PropagateDirectory *>(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 518bd9ec0299cdd3f8d8c91332c7442a60e9aa92..4b571609fa28033226f45496694948bb0abc3bd4 100644 (file)
--- 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 152d66b29119f113ba058456a3a86eafacfe456c..d479c15b6953380723cebee08125ac094a56622d 100644 (file)
--- 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;
         }