dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> copyjob.cpp:862
> + // if the destination file system doesn't support writing, do not
> stat
> + QFileInfo destInfo(m_currentDestURL.toString());
> + if ((m_mode == CopyJob::Copy || m_mode == CopyJob::Move) &&
> (destInfo.isDir() && destInfo.isWritable())) {
Wrong mix of URLs and paths. How can this ever work? QFileInfo takes local
paths, and you're giving it a full URL?
All this should be inside if (m_currentDestURL.isLocalFile()) and use
m_currentDestURL.toLocalFile() as the path to give to QFileInfo.
> copyjob.cpp:863
> + QFileInfo destInfo(m_currentDestURL.toString());
> + if ((m_mode == CopyJob::Copy || m_mode == CopyJob::Move) &&
> (destInfo.isDir() && destInfo.isWritable())) {
> + QPointer<CopyJob> that = q;
Why the "copy or move" test? The only alternative is creating a symlink, which
also requires being able to write, no?
> copyjob.cpp:865
> + QPointer<CopyJob> that = q;
> + emit q->warning(q, buildErrorString(ERR_CYCLIC_COPY,
> m_currentDestURL.toDisplayString()));
> + if (that) {
This used to be an error, now it gets degraded to a warning. This means
applications performing the copy will think it actually worked, only the user
got (maybe) a warning....
I think this should be an error.
And there should be a unittest for it.
JobTest::copyFolderWithUnaccessibleSubfolder shows how to make a folder
non-writable (and still be able to clean it up at the end). A similar test
should be added where the destination is the one that is unaccessible (or just
unwritable).
> broulik wrote in copyjob.cpp:864
> Not sure `ERR_CYCLIC_COPY` is the correct error for this?
It doesn't sound like it, no. This should rather be ERR_CANNOT_WRITE.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17545
To: shubham, #frameworks, dfaure
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns
