D28669: make CopyJob non-recursive

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.

INLINE COMMENTS

> copyjob.cpp:809
>      ++m_currentStatSrc;
> -    statCurrentSrc();
>  }

Why did you remove this call?

Your patch has no context (please use `arc` to upload patches to phabricator), but I can see here that this is in the CopyJobPrivate::skipSrc method. Did you test skipping files? This isn't mentioned in your test plan.

Does the unittest jobtest still pass?

> anthonyfieroni wrote in copyjob.cpp:814
> It'll not help, message queue will be blocked and you kill the from other thread, which is not what we want.

Which threads? There are no threads involved here.

There is no need to handling killing here. It wasn't any different in the orig code with the recursion.
As soon as we find actual work to do, we'll go and launch a subjob, at which point we go back to the event loop and can handle being killed.

> copyjob.cpp:849
>              files.append(info);   // Files and any symlinks
>              statNextSrc(); // we could use a loop instead of a recursive call :)
> +            continue;

This still recurses. How does this get rid of recursion? I don't get it.

> meven wrote in copyjob.cpp:892
> I guess we can update this comment

Why? it still recurses.

> copyjob.cpp:915
>          m_bURLDirty = true;
> -    } else {
> +    }
> +

This makes no sense to me. We are finished when the previous phase is actually finished.

REPOSITORY
  R241 KIO

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

To: McPain, #frameworks, dfaure, meven, ahmadsamir
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200408/3e3dc0d6/attachment-0001.html>