D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio abandoned this revision. dmitrio added a comment. No, didn't work on that, sorry. I think I am better to close the revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure, ngraham Cc: kde-frameworks-devel,

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. Any progress on that, dmitrio? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure, ngraham Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, bruns

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. Right, I think that makes sense. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure, ngraham Cc: kde-frameworks-devel, elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, bruns

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio added a comment. Well, I think it would be logical to implement something like that if we intend to make copying cancelable. But we still will have to spoil the destination file if there is no enough space left (which is not always determined correctly, in fact. One example of this

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. Restricted Application added a subscriber: kde-frameworks-devel. You're right, the destination file was indeed silently corrupted in the "before" case. Yikes, that's bad. Really, both options are bad. I can see now that your patch doesn't actually regress

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio added a comment. If I understand correctly what has happened, without this patch the file at destination path existed after the operation was cancelled, and it might even not be corrupted. Still I believe that it is the **source** file that may remain after the cancellation, but not

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

anthonyfieroni added inline comments. INLINE COMMENTS > filecopyjob.cpp:320 > +// It is better to clean up this file. > +if (!isSuspended() && d->m_bFileWritingIsInProcess) { > +KIO::file_delete(d->m_dest); && metadata(QStringLiteral("overwrite")) != QLatin1String("true")

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. Here, let me show you. Without your patch: F5829979: without the patch.webm With your patch: F5829981: with the patch.webm REPOSITORY R241 KIO REVISION DETAIL

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio added a comment. Sorry, but I was not able to locate such a behavior. When cancelling an overwrite in the middle of the process I got only a partially copied file at the destination path. Tested in the latest git-stable KDE Neon, the procedure was a copying of the source file to the

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. In D10663#254549 , @dmitrio wrote: > Then I probably didn't understand what was proposed in the discussed bug report. When you start writing to the existing file the data that have been in the destination file before

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio added a comment. Then I probably didn't understand what was proposed in the discussed bug report. When you start writing to the existing file the data that have been in the destination file before the operation become lost. If we want to be able to restore content of destination

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. When you cancel an overwrite operation in the middle with this patch, the destination file is destroyed. REPOSITORY R241 KIO REVISION DETAIL

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio added a comment. Yes, this code is ready for review. As I described in the previous comment, there is still a question concerning handling the full disk error case: it is being handled currently in some ioslaves (at least in `file_unix`

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. @dmitrio are you ready for review, or still working on this? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, bruns

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh, bruns

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio updated this revision to Diff 30570. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10663?vs=27582=30570 REVISION DETAIL https://phabricator.kde.org/D10663 AFFECTED FILES src/core/filecopyjob.cpp src/core/filecopyjob.h To: dmitrio, #frameworks,

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio reclaimed this revision. dmitrio added a comment. This revision now requires changes to proceed. Well, sorry, I did not expect you waiting for me. I had some new version of this patch, but thought about working on unit tests for it and never finished them. I'll upload now what I have

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. @dmitrio, are you still planning to work on this patch, or a new version of it? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure Cc: elvisangelaccio, ngraham, anthonyfieroni, meven, #frameworks, michaelh

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

elvisangelaccio added a comment. @dmitrio: you can click on the "Plan Changes" action, no need to abandon this revision and create a new one. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, dfaure Cc: elvisangelaccio, ngraham,

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dfaure added a comment. "the destination file gets deleted only if some data has been written into it" ... ah I see, that's a great solution indeed, I like it. About moving: if the file was fully copied, (but the source not yet deleted), good catch. This requires a similar solution in

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio abandoned this revision. dmitrio added a comment. In D10663#213898 , @dfaure wrote: > I don't see any provision for the case I mentioned, where the destination file already exists, and should therefore NOT be deleted? In fact,

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. In D10663#213898 , @dfaure wrote: > Better make it happen all the time, and better make it work right. A flag almost sounds like a excuse for a half-hearted feature ("if it works badly in case XYZ, then apps can just

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dfaure requested changes to this revision. dfaure added a comment. I don't like the idea of a flag for this in the API. It just moves the problem (of whether it's safe / a good idea to clean up) to the applications, who are not in a better place to decide about this. Better make it happen

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio planned changes to this revision. dmitrio added a comment. It seems that the proposed patch in its current state does not fully prevent possible problems with move operation (did not catch any problems while testing, but looking at the code again I don't think that all is good with

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added a comment. I would prefer to have the cleanup behavior happen by default, if we end up making this optional. It would be nice if we could also also handle the `KIO::ERR_DISK_FULL` case. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To:

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio marked an inline comment as done. dmitrio added a comment. In D10663#209708 , @meven wrote: > Add this to your commit message > FIXED-IN: 5.44 Thank you, added it to the description. In D10663#209711

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio updated this revision to Diff 27582. dmitrio edited the summary of this revision. dmitrio added a comment. Add a flag which turns off cleaning up on job cancel. Use m_currentDestURL instead of obtaining the destination file name from an iterator. REPOSITORY R241 KIO CHANGES

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

meven added a comment. It was suggested on IRC to hide this behavior behind a flag such as the jobFlag enum, so that it can be opt-in/opt-out in applications. We could consider changing the job state during the clean up, something like d->state == STATE_CLEANING_INCOMPLETE_FILE

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

anthonyfieroni edited reviewers, added: dfaure; removed: Dolphin. anthonyfieroni added a comment. It looks D10635 is duplicate. INLINE COMMENTS > copyjob.cpp:559 > +if (d->state == STATE_COPYING_FILES && d->m_bFileCopyingIsInProcess) { > +

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

meven added a comment. Add this to your commit message FIXED-IN: 5.44 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, #dolphin Cc: meven, #frameworks, michaelh

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, #dolphin Cc: #frameworks, michaelh

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

ngraham added reviewers: Frameworks, Dolphin. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10663 To: dmitrio, #frameworks, #dolphin Cc: #frameworks, michaelh

D10663: Remove a partially copied file if copyjob was cancelled in the middle of file copying

dmitrio created this revision. dmitrio added a project: Frameworks. dmitrio requested review of this revision. REVISION SUMMARY BUG: 383764 Remove a partially copied file if copyjob was cancelled in the middle of file copying. File is considered to be in the process of being copied