| dfaure | |
| ngraham |
Instead of truncating the existing file, a separate copy of the source file in the
destination folder is created. If the copy was successfully created, the existing
destination file is replaced with it. If in middle of creating the separate copy the
disk runs out of space then an attempt is made to delete the existing destination.
After that is done the copy operation is resumed.
BUG: 125102
FIXED-IN: 5.56
Unit testing and manual testing done.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Thanks for working on this, here's my review.
| autotests/jobtest.cpp | ||
|---|---|---|
| 1724 | This test is called Overwrite, and passes the Overwrite flag, but it's not actually overwriting anything (destFile doesn't exist). I think both cases should be tested --> make a _data() method with two rows, destFileExists=false and destFileExists=true. Another issue: the whole test needs to be skipped on Windows (use QSKIP in #ifdef), since you only implemented it in file_unix.cpp | |
| src/ioslaves/file/file_unix.cpp | ||
| 288 | This is all in the context of KIO::Overwrite being set, programatically. So the whole point *is* to overwrite without asking (this is often used after the user has already approved overwriting, or for cases where the user should not be involved like application-internal files). So it makes zero sense to me to ask the user about overwriting yet again. In fact the thing you're asking here is "do you agree that canceling before completion will no longer preserve the existing destination file, as it would do otherwise". In my opinion, this is too much detail, we shouldn't be asking a question "just in case the user wants to cancel". In all the cases where the user isn't going to cancel, this is really just an annoyance. I view the feature (no data loss during cancel-of-overwrite) as a nice bonus, but it's not a *bug* if an overwritten file is lost during an overwrite operation, by definition. So... I would suggest to proceed without asking. | |
| 414 | qCWarning supports <<, no need for arg(). qCWarning() << "Couldn't ..." << _dest_backup; | |
| 418 | (same) | |
Removed warning message.
Removed arg().
Added test cases.
Used QFile::resize().
Replying to a comment:
This test is called Overwrite, and passes the Overwrite flag, but it's not actually
overwriting anything (destFile doesn't exist).
createTestDirectory() internally calls createTestFile() so both source and dest
files are created. There, using QFile, I wanted to resize the source file to make
sure the .part file isn't deleted before the lambda is called.
Ah I see, we copy to .part only when the destination actually exists. I had missed that in my earlier comment. Makes sense, actually.
| src/ioslaves/file/file_unix.cpp | ||
|---|---|---|
| 289 | I think the naming of that bool could be improved (orig sounds like origin sounds like source, but this is about the dest). How about existing_dest_delete_attempted ? | |
| 322 | .constData() [everywhere you used .data()] | |
This test fails in CI, please investigate and fix:
FAIL! : JobTest::safeOverwrite(dest file exists) Compared values are not the same
Actual (spyTotalSize.count()): 0 Expected (1) : 1 Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/jobtest.cpp(1765)]
FAIL! : JobTest::safeOverwrite(dest file doesn't exist) Compared values are not the same
Actual (spyTotalSize.count()): 0 Expected (1) : 1 Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/jobtest.cpp(1765)]
| autotests/jobtest.cpp | ||
|---|---|---|
| 1759 | The other problems were fixed, but this test still fails randomly. In fact, why are we checking that the dest file already started to be created when totalSize is emitted? Maybe it's not totalSize we should connect to, but more of a progress signal.... | |
| autotests/jobtest.cpp | ||
|---|---|---|
| 1759 | ||