-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix moving folders out of a cache jail #5424
Conversation
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @d--j and @rullzer to be potential reviewers. |
Hello @icewind1991 How can i test this fix i don't have this path in my cloud : tests/lib/Files/Cache/Wrapper/CacheJailTest.php I have the CacheJail.php in lib/private/Files/Cache/Wrapper/ Does this CacheJailTest.php is necessary ? |
Codecov Report
@@ Coverage Diff @@
## master #5424 +/- ##
=============================================
- Coverage 54.15% 31.24% -22.91%
- Complexity 22349 22362 +13
=============================================
Files 1379 1380 +1
Lines 85557 85020 -537
Branches 1329 1329
=============================================
- Hits 46331 26564 -19767
- Misses 39226 58456 +19230
|
This is only the unit test, it is not packaged within the final releases. So you don't have to replace that file, it's only for our automated tests relevant :) |
@icewind1991 Can we have a repair step here? If possible a live repair step to not block the upgrade and in multiple smaller steps. |
after further testing seems to not work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change works for me locally, but we need to have a way to repair older installations. But there's another desktop client bug … let me try to reproduce this and file to the client repo 🙈
note that this issue is completely reproducible without the client by moving files from the web ui |
I have not yet been able to apply the fix but I wanted to report that this bug also happened when we create a shared folder. |
0b64df0
to
8a77b8d
Compare
@LukasReschke added repair step |
@nickvergessen Can you review the repair step? THX! |
@icewind1991 please write a unit test for the repair step. |
Signed-off-by: Robin Appelman <robin@icewind.nl>
8a77b8d
to
d3c20ee
Compare
@nickvergessen done |
I updated this here to do:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍 (also the repair step properly cleans it up)
|
||
public function run(IOutput $output) { | ||
$versionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); | ||
if (version_compare($versionFromBeforeUpdate, '13.0.0.1', '<')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we adjust this to either:
- Use an appconfig value
- Also check for 12.0.1 as we want to backport this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use an appconfig value
This needs to be cleaned up later.
Also check for 12.0.1 as we want to backport this one :)
Okay - if this is 12.0.1 then I will add it as well.
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
c0af593
to
846e62c
Compare
|
||
use OC\Files\Cache\Cache; | ||
use OC\Files\Storage\Temporary; | ||
use OC\Repair\RepairInvalidPaths; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test not passing, you moved that file 😉
Seems to work ok for moving folders/files out of a cache jail, but still seeing some occurrences of duplicate files as #3502. Tested on 12.0.0.29 - manually backported from changed files. |
Signed-off-by: Robin Appelman <robin@icewind.nl>
Note that it results in lost data 😢 😱 |
@LukasReschke please re-review |
Tried to reproduce #3502 with linux client (2.3.2beta, build 13) and this branch. No can do. Works fine for me. |
Seeing as 👍 -> merging |
@icewind1991 please create the backport PR ;) |
Fixes #3502
@plrunner @mrieser @bat553 please verify if this fixes the problem for you