-
-
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
Do not calculate folder size for parent that also needs proper scan, fixes #3524 #14425
Do not calculate folder size for parent that also needs proper scan, fixes #3524 #14425
Conversation
dc3308c
to
71c78a0
Compare
@icewind1991 @rullzer The case I am talking about is that when two folders are marked with size -1 and share some part of its path. Like /Mainfolder and /Mainfolder/Subfolder. Only Subfolder will be scanned correctly and checked for modified files. To reproduce this you can simply do this 3 commands:
In a terminal with the SMB share mounted normally with cifs outside of nextcloud:
In this simple example, only Subfolder along with subtest.txt will have its new size. Mainfolder will have its old size + Subfolders change in size. test.txt will never be scanned. Let me know if you have any questions :) |
Alright so I fixed the bug a more proper way I believe. Please review. Though I forgot to sign my revert before I commited the newer fix. |
dc34064
to
eb6e8fe
Compare
@icewind1991 Could you check out the new changes if that could be merged? I am wondering if the changes breaks other stuff. |
One case that is not handled efficiently is when two folders both have size = -1 and shares parent. |
Update: I pushed more code to handle this scenario: Masterfolder When there is a change in both test1.txt and test2.txt, avoid scanning the whole Masterfolder. |
This are failing testShallow tests right now. I need to make it only avoid doing correctFolderSize() on parent in a backgroundscan. @icewind1991 Would you accept adding a third parameter to correctFolderSize():
I have tried that and the tests works then. |
Makes sense to me 👍 Also the usage is not limiting it, because it's internally used only. |
Could you then squash all your commits? |
0f294c6
to
e5272e3
Compare
e5272e3
to
8605453
Compare
8605453
to
77b8c47
Compare
Signed-off-by: Ari Selseng <ari@selseng.net>
77b8c47
to
d16cfb5
Compare
@MorrisJobke I added a test now to make sure this bug never comes back again. How does it look? |
The tests look really awesome \o/ 👍 |
Alright let's get this reviewed and merged :) |
@rullzer @icewind1991 |
Status of 16825: failureDB=sqlite, ENABLE_REDIS=false, PHP=7.3Show full log
DB=mysqlmb4, ENABLE_REDIS=false, PHP=7.3Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-comments
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-files
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-files-tags
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=login
Show full log
|
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.
Code looks good 👍
/backport to stable15 |
/backport to stable14 |
backport to stable14 in #14598 |
backport to stable15 in #14597 |
As discussed in #3524
This small patch avoids a weird edge case when multiple incomplete folders shares some of their tree and fileid is higher on the lowest folder.
Example:
testfile1.txt and testfile2.txt gets modified and occ files_external:notify registers it by setting size -1 on subfolder1 and subfolder2.
Tree:
subfolder1/testfile1.txt
subfolder1/subfolder2/testfile2.txt
If subfolder2 has higher fileid, only testfile1.txt would be properly scanned when doing a backgroundScan. subfolder1 would just get its old cached size back and not properly scan
Any thoughts? @MorrisJobke @icewind1991
Any chance this could be backported to 15? It seems to be it should be a quite a top priority bug.