SyncEngine: Fix renames making hierarchy inversion #6695
Conversation
ogoffart
changed the title
test/testsyncmove.cpp
Outdated
| QCOMPARE(fakeFolder.currentLocalState(), expectedState); | ||
| QCOMPARE(fakeFolder.currentRemoteState(), expectedState); | ||
|
|
||
| /* FIXME |
There was a problem hiding this comment.
Yes, this test is still failing. Fixing it would be quite some work and i think this can wait 2.6
There was a problem hiding this comment.
Agree, since it hasn't come up before...
src/libsync/syncengine.cpp
Outdated
| auto original_it = _renamedFolders.constFind(original); | ||
| if (original_it != _renamedFolders.constEnd() && it->startsWith(*original_it)) { | ||
| return original; // Issue #6694: "hirarchy inversion" | ||
| } |
There was a problem hiding this comment.
I don't get this logic. Are you sure it doesn't indicate we should reverse the order of traversing paths in the while?
As far as I understand (haven't tested!) our renamedFolders will have two entries if "/Documents/Empty" is renamed to "/Empty/Documents":
/Documents -> /Empty/Documents
/Documents/Empty -> /Empty
Previously the function should have mapped "/Documents/Empty" to "/Empty/Documents/Empty" because the first parent directory rename is appied. With your change it'll be "/Empty/Documents" because the full path rename is taken instead.
There are probably corner cases now with three-level paths? Why aren't we taking the longest applicable rename path instead of the shortest?
(if the current logic is correct, I'd like more comments explaining it)
There was a problem hiding this comment.
Previously the function should have mapped "/Documents/Empty" to "/Empty/Documents/Empty" because the first parent directory rename is appied.
Yes, that's correct and that was the bug.
But with my change it will return the unchanged original .
Yes, there will be problem with three-level path when each level is renamed, or with cycle that involve several folder (as the FIXME test is testing). But this fix for that is more complicated.
There was a problem hiding this comment.
Ok! Then the problem was that there was no indication that this was just fixing the most common problem in time for 2.5. Could you add something along the lines of // band-aid to fix a common problem, there's a larger issue here with ... like in example ...?
There was a problem hiding this comment.
This fix is only for the particualr case in issue #6694, but the problem is actually much bigger and a bigger change is required which i do not think is in the scope for 2.5
Should I discard this change completely and only make a good fix in 2.6 ?
test/testsyncmove.cpp
Outdated
| QCOMPARE(fakeFolder.currentLocalState(), expectedState); | ||
| QCOMPARE(fakeFolder.currentRemoteState(), expectedState); | ||
|
|
||
| /* FIXME |
There was a problem hiding this comment.
Yes, this test is still failing. Fixing it would be quite some work and i think this can wait 2.6
src/libsync/syncengine.cpp
Outdated
| auto original_it = _renamedFolders.constFind(original); | ||
| if (original_it != _renamedFolders.constEnd() && it->startsWith(*original_it)) { | ||
| return original; // Issue #6694: "hirarchy inversion" | ||
| } |
There was a problem hiding this comment.
Previously the function should have mapped "/Documents/Empty" to "/Empty/Documents/Empty" because the first parent directory rename is appied.
Yes, that's correct and that was the bug.
But with my change it will return the unchanged original .
Yes, there will be problem with three-level path when each level is renamed, or with cycle that involve several folder (as the FIXME test is testing). But this fix for that is more complicated.
Issue #6694
This is not a problem in the
new_discovery_algobranch, but let's fix that for 2.5