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

[Ahmad Samir]

ahmadsamir marked 2 inline comments as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in copyjob.cpp:430
> 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.

Copy-paste-comment logic mistake.

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

path as in "destination dir", but indeed technically it's not the 'path' part for remote urls. And toDisplayString will remove any passwords (not sure there may be passwords here, but still, safer that way).

> dfaure wrote in copyjob.cpp:444
> 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.

Excellent points; I was scratching my head trying to figure out what use UDS_LOCAL_PATH is when, while testing with qDebug(), it was _always_ empty, the only time it wasn't empty was in testtrash unit test, some code there set it, used it or something. So I resorted to grep'ing the code and I found the KCoreDirLister::cachedItemForUrl instance (which answers your next comment). So someone who knows all the use-cases of UDS_LOCAL_PATH (you in this case) should/must update the UDS_LOCAL_PATH docs :D

> dfaure wrote in copyjob.cpp:478
> 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)

"The lambda is called after going back to the event loop, so this will have happened anyway, no?"
Good point (thanks for the info, I didn't think of the event loop POV).

However, right after the connect(), 'return' is called, so as not to call statCurrentSrc() on line 502, I want it called from the lambda _after_ the free space check job finishes. In one of the iterations working on this patch I got a race, where it would sometimes work and error out if there isn't enough free space at a remote dest, but some other times it would start copying; hence the "must" part; I don't recall the exact details though (I've been working on it for a couple of days, I tried so many stuff, the details of the trial and error are mushed all together at this point.... sorry :/)

So, I'll remove the comment or improve it to convey the idea in a clearer way.

> copyjob.cpp:479
> +        // Must do this here before statCurrentSrc() is called in the lambda connected to
> +        // FileSystemFreeSpaceJob below
>          q->removeSubjob(job);

s/below/below, because return is called right after it, so that statCurrentSrc is called from the lambda/

does that sound better?

> dfaure wrote in copyjob.cpp:485
> 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.

Looking at it again, removeSubjob can be called before the free disk space check, because it's statCurrentSrc that adds a new subJob (I was worried of calling removeSubjob too early).

> dfaure wrote in copyjob.cpp:486
> ... 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 ?

actualDest sounds nicer.

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/b907e65f/attachment-0001.htm>