[Pkg-owncloud-commits] [owncloud-client] 07/09: Add patches to fix SyncMoveTest
Sandro Knauß
hefee at debian.org
Wed Dec 6 19:09:48 UTC 2017
This is an automated email from the git hooks/post-receive script.
hefee pushed a commit to branch experimental
in repository owncloud-client.
commit 6e452760aa8c6ea615b30afb958a2a553f7308a1
Author: Sandro Knauß <hefee at debian.org>
Date: Wed Dec 6 18:18:14 2017 +0100
Add patches to fix SyncMoveTest
---
...ear-csync-rename-mappings-after-reconcile.patch | 27 ++
...ename-maps-are-consistent-with-update-pha.patch | 294 +++++++++++++++++++++
debian/patches/series | 2 +
3 files changed, 323 insertions(+)
diff --git a/debian/patches/0010-Clear-csync-rename-mappings-after-reconcile.patch b/debian/patches/0010-Clear-csync-rename-mappings-after-reconcile.patch
new file mode 100644
index 0000000..b8bdd37
--- /dev/null
+++ b/debian/patches/0010-Clear-csync-rename-mappings-after-reconcile.patch
@@ -0,0 +1,27 @@
+From 99f32dcb99c6f2f99914aa5b4ec254ba0d48e3ba Mon Sep 17 00:00:00 2001
+From: Christian Kamm <mail at ckamm.de>
+Date: Tue, 5 Dec 2017 13:41:27 +0100
+Subject: [PATCH 1/2] Clear csync rename mappings after reconcile
+
+They were being preserved *across sync runs*.
+---
+ src/csync/csync.cpp | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/src/csync/csync.cpp b/src/csync/csync.cpp
+index 4f0c19916..e6291f427 100644
+--- a/src/csync/csync.cpp
++++ b/src/csync/csync.cpp
+@@ -314,6 +314,9 @@ int csync_s::reinitialize() {
+ local.files.clear();
+ remote.files.clear();
+
++ renames.folder_renamed_from.clear();
++ renames.folder_renamed_to.clear();
++
+ status = CSYNC_STATUS_INIT;
+ SAFE_FREE(error_string);
+
+--
+2.15.1
+
diff --git a/debian/patches/0011-Reconcile-Rename-maps-are-consistent-with-update-pha.patch b/debian/patches/0011-Reconcile-Rename-maps-are-consistent-with-update-pha.patch
new file mode 100644
index 0000000..9a2fe31
--- /dev/null
+++ b/debian/patches/0011-Reconcile-Rename-maps-are-consistent-with-update-pha.patch
@@ -0,0 +1,294 @@
+From ceac18c5547836f2b4d6e1410adfbd1f0aaa1bee Mon Sep 17 00:00:00 2001
+From: Christian Kamm <mail at ckamm.de>
+Date: Tue, 5 Dec 2017 13:42:55 +0100
+Subject: [PATCH 2/2] Reconcile: Rename maps are consistent with update phase
+ #6212
+
+For duplicate file ids the update phase and reconcile phase determined
+the rename mappings independently. If they disagreed (due to different
+order of processing), complicated misbehavior would result.
+
+This patch fixes it by letting reconcile try to use the mapping that the
+update phase has computed first.
+---
+ src/csync/csync.cpp | 4 ++--
+ src/csync/csync_reconcile.cpp | 47 +++++++++++++++++++++++++++++++-----------
+ src/csync/csync_rename.cpp | 20 +++++++++++++++---
+ src/csync/csync_rename.h | 15 +++++++++++---
+ src/libsync/discoveryphase.cpp | 2 +-
+ test/testsyncmove.cpp | 27 +++++++++++++++++-------
+ 6 files changed, 87 insertions(+), 28 deletions(-)
+
+diff --git a/src/csync/csync.cpp b/src/csync/csync.cpp
+index e6291f427..93438007a 100644
+--- a/src/csync/csync.cpp
++++ b/src/csync/csync.cpp
+@@ -211,14 +211,14 @@ static int _csync_treewalk_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
+
+ if (other_file_it == other_tree->cend()) {
+ /* Check the renamed path as well. */
+- QByteArray renamed_path = csync_rename_adjust_path(ctx, cur->path);
++ QByteArray renamed_path = csync_rename_adjust_parent_path(ctx, cur->path);
+ if (renamed_path != cur->path)
+ other_file_it = other_tree->find(renamed_path);
+ }
+
+ if (other_file_it == other_tree->cend()) {
+ /* Check the source path as well. */
+- QByteArray renamed_path = csync_rename_adjust_path_source(ctx, cur->path);
++ QByteArray renamed_path = csync_rename_adjust_parent_path_source(ctx, cur->path);
+ if (renamed_path != cur->path)
+ other_file_it = other_tree->find(renamed_path);
+ }
+diff --git a/src/csync/csync_reconcile.cpp b/src/csync/csync_reconcile.cpp
+index b2283ad2c..97863fb8b 100644
+--- a/src/csync/csync_reconcile.cpp
++++ b/src/csync/csync_reconcile.cpp
+@@ -113,7 +113,7 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
+
+ if (!other) {
+ /* Check the renamed path as well. */
+- other = other_tree->findFile(csync_rename_adjust_path(ctx, cur->path));
++ other = other_tree->findFile(csync_rename_adjust_parent_path(ctx, cur->path));
+ }
+ if (!other) {
+ /* Check if it is ignored */
+@@ -147,24 +147,25 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
+ cur->instruction = CSYNC_INSTRUCTION_NEW;
+
+ bool processedRename = false;
+- auto renameCandidateProcessing = [&](const OCC::SyncJournalFileRecord &base) {
++ auto renameCandidateProcessing = [&](const QByteArray &basePath) {
+ if (processedRename)
+ return;
+- if (!base.isValid())
++ if (basePath.isEmpty())
+ return;
+
+ /* First, check that the file is NOT in our tree (another file with the same name was added) */
+- if (our_tree->findFile(base._path)) {
+- qCDebug(lcReconcile, "Origin found in our tree : %s", base._path.constData());
++ if (our_tree->findFile(basePath)) {
++ other = nullptr;
++ qCDebug(lcReconcile, "Origin found in our tree : %s", basePath.constData());
+ } else {
+ /* Find the potential rename source file in the other tree.
+ * If the renamed file could not be found in the opposite tree, that is because it
+ * is not longer existing there, maybe because it was renamed or deleted.
+ * The journal is cleaned up later after propagation.
+ */
+- other = other_tree->findFile(base._path);
++ other = other_tree->findFile(basePath);
+ qCDebug(lcReconcile, "Rename origin in other tree (%s) %s",
+- base._path.constData(), other ? "found" : "not found");
++ basePath.constData(), other ? "found" : "not found");
+ }
+
+ if(!other) {
+@@ -197,7 +198,7 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
+ cur->instruction = CSYNC_INSTRUCTION_NONE;
+ // We have consumed 'other': exit this loop to not consume another one.
+ processedRename = true;
+- } else if (our_tree->findFile(csync_rename_adjust_path(ctx, other->path)) == cur) {
++ } else if (our_tree->findFile(csync_rename_adjust_parent_path(ctx, other->path)) == cur) {
+ // If we're here, that means that the other side's reconcile will be able
+ // to work against cur: The filename itself didn't change, only a parent
+ // directory was renamed! In that case it's safe to ignore the rename
+@@ -225,12 +226,34 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) {
+ qCDebug(lcReconcile, "Finding rename origin through inode %" PRIu64 "",
+ cur->inode);
+ ctx->statedb->getFileRecordByInode(cur->inode, &base);
+- renameCandidateProcessing(base);
++ renameCandidateProcessing(base._path);
+ } else {
+ ASSERT(ctx->current == REMOTE_REPLICA);
+- qCDebug(lcReconcile, "Finding rename origin through file ID %s",
+- cur->file_id.constData());
+- ctx->statedb->getFileRecordsByFileId(cur->file_id, renameCandidateProcessing);
++
++ // The update phase has already mapped out all dir->dir renames, check the
++ // path that is consistent with that first. Otherwise update mappings and
++ // reconcile mappings might disagree, leading to odd situations down the
++ // line.
++ auto basePath = csync_rename_adjust_full_path_source(ctx, cur->path);
++ if (basePath != cur->path) {
++ qCDebug(lcReconcile, "Trying rename origin by csync_rename mapping %s",
++ basePath.constData());
++ // We go through getFileRecordsByFileId to ensure the basePath
++ // computed in this way also has the expected fileid.
++ ctx->statedb->getFileRecordsByFileId(cur->file_id,
++ [&](const OCC::SyncJournalFileRecord &base) {
++ if (base._path == basePath)
++ renameCandidateProcessing(basePath);
++ });
++ }
++
++ // Also feed all the other files with the same fileid if necessary
++ if (!processedRename) {
++ qCDebug(lcReconcile, "Finding rename origin through file ID %s",
++ cur->file_id.constData());
++ ctx->statedb->getFileRecordsByFileId(cur->file_id,
++ [&](const OCC::SyncJournalFileRecord &base) { renameCandidateProcessing(base._path); });
++ }
+ }
+
+ break;
+diff --git a/src/csync/csync_rename.cpp b/src/csync/csync_rename.cpp
+index da76cff52..80b3f1439 100644
+--- a/src/csync/csync_rename.cpp
++++ b/src/csync/csync_rename.cpp
+@@ -36,7 +36,7 @@ void csync_rename_record(CSYNC* ctx, const QByteArray &from, const QByteArray &t
+ ctx->renames.folder_renamed_from[to] = from;
+ }
+
+-QByteArray csync_rename_adjust_path(CSYNC* ctx, const QByteArray &path)
++QByteArray csync_rename_adjust_parent_path(CSYNC *ctx, const QByteArray &path)
+ {
+ if (ctx->renames.folder_renamed_to.empty())
+ return path;
+@@ -50,11 +50,25 @@ QByteArray csync_rename_adjust_path(CSYNC* ctx, const QByteArray &path)
+ return path;
+ }
+
+-QByteArray csync_rename_adjust_path_source(CSYNC* ctx, const QByteArray &path)
++QByteArray csync_rename_adjust_parent_path_source(CSYNC *ctx, const QByteArray &path)
+ {
+ if (ctx->renames.folder_renamed_from.empty())
+ return path;
+- for (auto p = _parentDir(path); !p.isEmpty(); p = _parentDir(p)) {
++ for (ByteArrayRef p = _parentDir(path); !p.isEmpty(); p = _parentDir(p)) {
++ auto it = ctx->renames.folder_renamed_from.find(p);
++ if (it != ctx->renames.folder_renamed_from.end()) {
++ QByteArray rep = it->second + path.mid(p.length());
++ return rep;
++ }
++ }
++ return path;
++}
++
++QByteArray csync_rename_adjust_full_path_source(CSYNC *ctx, const QByteArray &path)
++{
++ if (ctx->renames.folder_renamed_from.empty())
++ return path;
++ for (ByteArrayRef p = path; !p.isEmpty(); p = _parentDir(p)) {
+ auto it = ctx->renames.folder_renamed_from.find(p);
+ if (it != ctx->renames.folder_renamed_from.end()) {
+ QByteArray rep = it->second + path.mid(p.length());
+diff --git a/src/csync/csync_rename.h b/src/csync/csync_rename.h
+index 194f4256c..825f45f4e 100644
+--- a/src/csync/csync_rename.h
++++ b/src/csync/csync_rename.h
+@@ -22,10 +22,19 @@
+
+ #include "csync.h"
+
+-/* Return the final destination path of a given patch in case of renames */
+-QByteArray OCSYNC_EXPORT csync_rename_adjust_path(CSYNC *ctx, const QByteArray &path);
++/* Return the final destination path of a given patch in case of renames
++ *
++ * Does only map the parent directories. If the directory "A" is renamed to
++ * "B" then this function will not map "A" to "B". Only "A/foo" -> "B/foo".
++*/
++QByteArray OCSYNC_EXPORT csync_rename_adjust_parent_path(CSYNC *ctx, const QByteArray &path);
++
+ /* Return the source of a given path in case of renames */
+-QByteArray OCSYNC_EXPORT csync_rename_adjust_path_source(CSYNC *ctx, const QByteArray &path);
++QByteArray OCSYNC_EXPORT csync_rename_adjust_parent_path_source(CSYNC *ctx, const QByteArray &path);
++
++/* like the parent_path variant, but applying to the full path */
++QByteArray OCSYNC_EXPORT csync_rename_adjust_full_path_source(CSYNC *ctx, const QByteArray &path);
++
+ void OCSYNC_EXPORT csync_rename_record(CSYNC *ctx, const QByteArray &from, const QByteArray &to);
+ /* Return the amount of renamed item recorded */
+ bool OCSYNC_EXPORT csync_rename_count(CSYNC *ctx);
+diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp
+index 8f99bc66f..14df57ac3 100644
+--- a/src/libsync/discoveryphase.cpp
++++ b/src/libsync/discoveryphase.cpp
+@@ -75,7 +75,7 @@ bool DiscoveryJob::isInSelectiveSyncBlackList(const QByteArray &path) const
+
+ // Also try to adjust the path if there was renames
+ if (csync_rename_count(_csync_ctx)) {
+- QByteArray adjusted = csync_rename_adjust_path_source(_csync_ctx, path);
++ QByteArray adjusted = csync_rename_adjust_parent_path_source(_csync_ctx, path);
+ if (adjusted != path) {
+ return findPathInList(_selectiveSyncBlackList, QString::fromUtf8(adjusted));
+ }
+diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
+index 11b354ad4..5049b019a 100644
+--- a/test/testsyncmove.cpp
++++ b/test/testsyncmove.cpp
+@@ -231,12 +231,25 @@ private slots:
+ QCOMPARE(fakeFolder.currentLocalState(), remoteInfo);
+ }
+
++ void testDuplicateFileId_data()
++ {
++ QTest::addColumn<QString>("prefix");
++
++ // There have been bugs related to how the original
++ // folder and the folder with the duplicate tree are
++ // ordered. Test both cases here.
++ QTest::newRow("first ordering") << "O"; // "O" > "A"
++ QTest::newRow("second ordering") << "0"; // "0" < "A"
++ }
++
+ // If the same folder is shared in two different ways with the same
+ // user, the target user will see duplicate file ids. We need to make
+ // sure the move detection and sync still do the right thing in that
+ // case.
+ void testDuplicateFileId()
+ {
++ QFETCH(QString, prefix);
++
+ FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+ auto &remote = fakeFolder.remoteModifier();
+
+@@ -245,8 +258,8 @@ private slots:
+ remote.mkdir("A/Q");
+
+ // Duplicate every entry in A under O/A
+- remote.mkdir("O");
+- remote.children["O"].addChild(remote.children["A"]);
++ remote.mkdir(prefix);
++ remote.children[prefix].addChild(remote.children["A"]);
+
+ // This already checks that the rename detection doesn't get
+ // horribly confused if we add new files that have the same
+@@ -263,28 +276,28 @@ private slots:
+
+ // Try a remote file move
+ remote.rename("A/a1", "A/W/a1m");
+- remote.rename("O/A/a1", "O/A/W/a1m");
++ remote.rename(prefix + "/A/a1", prefix + "/A/W/a1m");
+ QVERIFY(fakeFolder.syncOnce());
+ QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+ QCOMPARE(nGET, 0);
+
+ // And a remote directory move
+ remote.rename("A/W", "A/Q/W");
+- remote.rename("O/A/W", "O/A/Q/W");
++ remote.rename(prefix + "/A/W", prefix + "/A/Q/W");
+ QVERIFY(fakeFolder.syncOnce());
+ QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+ QCOMPARE(nGET, 0);
+
+ // Partial file removal (in practice, A/a2 may be moved to O/a2, but we don't care)
+- remote.rename("O/A/a2", "O/a2");
++ remote.rename(prefix + "/A/a2", prefix + "/a2");
+ remote.remove("A/a2");
+ QVERIFY(fakeFolder.syncOnce());
+ QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+ QCOMPARE(nGET, 0);
+
+ // Local change plus remote move at the same time
+- fakeFolder.localModifier().appendByte("O/a2");
+- remote.rename("O/a2", "O/a3");
++ fakeFolder.localModifier().appendByte(prefix + "/a2");
++ remote.rename(prefix + "/a2", prefix + "/a3");
+ QVERIFY(fakeFolder.syncOnce());
+ QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+ QCOMPARE(nGET, 1);
+--
+2.15.1
+
diff --git a/debian/patches/series b/debian/patches/series
index dd3cdd0..2ca8671 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -7,3 +7,5 @@
0007-move-translations.patch
0008-make-reproducable.patch
0009-fix-installpath-of-dolphin-plugin.patch
+0010-Clear-csync-rename-mappings-after-reconcile.patch
+0011-Reconcile-Rename-maps-are-consistent-with-update-pha.patch
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud-client.git
More information about the Pkg-owncloud-commits
mailing list