D17545: Do not stat move/copy job if the destination file system does not support writing

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181213/68018346/attachment-0001.html>