[Pkg-owncloud-commits] [owncloud-client] 41/60: Reconcile: Rename maps are consistent with update phase #6212
Sandro Knauß
hefee at debian.org
Sat Dec 16 10:38:14 UTC 2017
This is an automated email from the git hooks/post-receive script.
hefee pushed a commit to branch upstream
in repository owncloud-client.
commit ceac18c5547836f2b4d6e1410adfbd1f0aaa1bee
Author: Christian Kamm <mail at ckamm.de>
Date: Tue Dec 5 13:42:55 2017 +0100
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 e6291f4..9343800 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 b2283ad..97863fb 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 da76cff..80b3f14 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 194f425..825f45f 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 8f99bc6..14df57a 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 11b354a..5049b01 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);
--
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