<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D18904">View Revision</a></tr></table><br /><div><div><p>Withdrawing my approval, after realizing that this is missing the "enough free space at destination" check.<br />
Imagine a 5GB partition with a 4GB file on it. You want to copy another version of that 4GB file onto it. Old implementation: it works. New implementation: we start copying 1GB into a .part file, then it fails (out of space), and then IIUC we finally do the right thing and overwrite the file. Right? So it ends up working, but it's much slower than it should behave. This could be improved with a free space check, right?</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D18904#inline-109501">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filecopyjob.cpp:479</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">job</span> <span style="color: #aa2211">==</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_copyJob</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_bFileWriteInProgress</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"file write" sounds like putJob, which isn't what this is about.</p>

<p style="padding: 0; margin: 8px;">I guess this should be called d->m_bFileCopyInProgress instead.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D18904">https://phabricator.kde.org/D18904</a></div></div><br /><div><strong>To: </strong>chinmoyr, dfaure, dmitrio, ngraham<br /><strong>Cc: </strong>kde-frameworks-devel, ngraham, michaelh, bruns<br /></div>