<table><tr><td style="">ahmadsamir added inline comments.
</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/D29610">View Revision</a></tr></table><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/D29610#inline-170119">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:1052</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'm confused. We want to solve renaming 'a' to 'A' when 'A' does *not* exist.</p>
<p style="padding: 0; margin: 8px;">QFile::rename will not overwrite an existing file, so it will do nothing if dest exists.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">const QByteArray dest1 = "/mnt/fat32/A";
const QByteArray dest2 = "/mnt/fat32/a";
QT_STATBUF buff_dest;
qDebug() << QT_LSTAT(dest1, &buff_dest);
qDebug() << QT_LSTAT(dest2, &buff_dest);</pre></div>
<p style="padding: 0; margin: 8px;">IIUC, from FAT32 POV, 'A' exists if 'a' exists and vice versa.</p></div></div><br /><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/D29610#inline-170168">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:1074</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Wouldn't it be enough to just call QFile::rename here?</p>
<p style="padding: 0; margin: 8px;">The whole idea is: if QFile::rename is able to rename a file in all cases, including the a->A special case on FAT, then let's just delegate the renaming to QFile.</p>
<p style="padding: 0; margin: 8px;">Then we don't need to have any special case in our code.</p>
<p style="padding: 0; margin: 8px;">QFile::rename does not overwrite, though, so if the dest exists and the Overwrite flag is set, we might have to either delete the dest first (race condition, not sure it matters here), or in *that* case use ::rename() since we know it can't be a FAT32-case-change (FAT32 can't have both a and A).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Too many quirks if we don't use ::rename(). I tested with e.g. in file_unix.cpp:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">bool renamed = QFile::rename(src, dest);
if (!renamed) {
if ((_flags & KIO::Overwrite) || isSrcSymLink) {
renamed = ::rename(_src.data(), _dest.data()) == 0;
}
}</pre></div>
<ul class="remarkup-list">
<li class="remarkup-list-item">QFile::rename doesn't overwrite as you said</li>
<li class="remarkup-list-item">renaming symlinks depends on ::rename</li>
<li class="remarkup-list-item">some unit tests are failing due to permissions</li>
</ul>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">FAIL! : JobTest::statDetailsBasic() Compared values are not the same
Actual (kioItem.permissions()): 420
Expected (438) : 438
Loc: [/home/ahmad/dev/kio/autotests/jobtest.cpp(1464)]
INFO : JobTest::statDetailsBasicSetDetails() entering
FAIL! : JobTest::statDetailsBasicSetDetails() Compared values are not the same
Actual (kioItem.permissions()): 420
Expected (438) : 438
Loc: [/home/ahmad/dev/kio/autotests/jobtest.cpp(1503)]</pre></div>
<p style="padding: 0; margin: 8px;">That and ::rename() is used all over the place in file_unix.cpp, I am not that comfortable using QFile::rename except for the freaky FAT32 case :)</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/D29610">https://phabricator.kde.org/D29610</a></div></div><br /><div><strong>To: </strong>ahmadsamir, Frameworks, dfaure<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>