D29485: [CopyJob] Check free space for remote urls before copying and other improvements

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Good idea overall.

INLINE COMMENTS

> copyjob.cpp:430
> +            if (!m_privilegeExecutionEnabled && !isWritable) {
> +                // In copy-as mode, we want to check the directory to which we're copying.
> +                // The target file or directory does not exist yet.

Here you kept a comment that said "we want to check", but the check already happened.
I'd say just remove the two lines of comments.
The code is clearer without them.

> copyjob.cpp:433
> +                const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +                const QString path = dest.isLocalFile() ? dest.toLocalFile() : dest.toString();
> +                q->setError(ERR_WRITE_ACCESS_DENIED);

That is not a path, for remote URLs. I suggest using dest.toDisplayString(QUrl::PreferLocalFiles) and just inlining that in the setErrorText call.

> copyjob.cpp:444
>  
> -            const QString sLocalPath = entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH);
> -            if (!sLocalPath.isEmpty() && kio_resolve_local_urls) {
> -                const QString fileName = m_dest.fileName();
> -                m_dest = QUrl::fromLocalFile(sLocalPath);
> -                if (m_asMethod) {
> -                    m_dest = addPathToUrl(m_dest, fileName);
> +            if (m_dest.isLocalFile()) {
> +                // Fast code path, if a dir is already in the kcoredirlister cache and it was e.g.

I think this check is too early.

UDS_LOCAL_PATH is also set by kio_desktop and kio_remote, for instance.
It's the main point of that entry: to map URLs from kioslaves-that-wrap-the-local-file-system back to local paths.

AFAICS the old code would use UDS_LOCAL_PATH also for non-local-file URLs.

> copyjob.cpp:445
> +            if (m_dest.isLocalFile()) {
> +                // Fast code path, if a dir is already in the kcoredirlister cache and it was e.g.
> +                // renamed and got UDS_LOCAL_PATH set

I completely fail to see the relation between this comment and the code.

"If a dir is already in kcoredirlister" is only relevant when calling KCoreDirLister::cachedItemForUrl, but that's not done here.

> copyjob.cpp:460
>                  }
> -                qCDebug(KIO_COPYJOB_DEBUG) << "Setting m_dest to the local path:" << sLocalPath;
> -                if (isGlobalDest) {
> -                    m_globalDest = m_dest;
> +
> +                const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

The check for m_dest.isLocalFile() goes here.

Given line 451, maybe it wasn't local initially, and it is now.

> copyjob.cpp:461
> +
> +                const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +                const QString path = dest.toLocalFile();

Now *this* is where the comment about "we want to check..." belongs :-)

The explanation of why we go "up" in case of copyAs, in order not to confuse KDiskFreeSpaceInfo.

> copyjob.cpp:465
> +                // Check available free space for local urls
> +                KDiskFreeSpaceInfo freeSpaceInfo =KDiskFreeSpaceInfo::freeSpaceInfo(path);
> +                if (freeSpaceInfo.isValid()) {

missing space after =

> copyjob.cpp:478
>  
> +        // Must do this here before statCurrentSrc() is called in the lambda connected to
> +        // FileSystemFreeSpaceJob below

The lambda is called after going back to the event loop, so this will have happened anyway, no?
I don't get it.

On the other hand I'm fine if this is done here, I'm just not sure why the comment says it has to be so.

(Easy solution is to remove that comment, especially given the suggestion below it won't even seem weird to do things in this order)

> copyjob.cpp:485
> +        // TODO: find a way to report connection errors to the user
> +        if (!m_dest.isLocalFile()) {
> +            const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : m_dest;

Maybe you can move the if (m_dest.isLocalFile()) block here, and use `else`.
It'll be clearer that free space check happens in both cases.

> copyjob.cpp:486
> +        if (!m_dest.isLocalFile()) {
> +            const QUrl dest = m_asMethod ? m_dest.adjusted(QUrl::RemoveFilename) : m_dest;
> +            KIO::FileSystemFreeSpaceJob *spaceJob = KIO::fileSystemFreeSpace(dest);

... and this line is the same in both code paths, so it could be extracted to before the if().

To avoid confusion between dest and m_dest, maybe rename this var to destToCheck ?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29485

To: ahmadsamir, #frameworks, dfaure, meven, sitter
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200508/73bede5d/attachment-0001.htm>