<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112481/">http://git.reviewboard.kde.org/r/112481/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 3rd, 2013, 4:35 p.m. CEST, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It's now kioslave/file/file.cpp, please adjust indeed.
Don't simplify isReadable: there's 000 (the first if), 444 (the second if), and other combinations (004, 040, 400) (the case where neither of these simple tests pass so we must use additional API to check for readability).
You suggestion "a 4 anywhere" doesn't make the file necessarily readable to us (if we don't own it).</pre>
</blockquote>
<p>On September 3rd, 2013, 5:01 p.m. CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">But then the second if should be "(S_IRUSR|S_IRGRP|S_IROTH) == d->m_permissions", not "(S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions", no?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Actually rather "((S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions) == (S_IRUSR|S_IRGRP|S_IROTH))"</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On September 3rd, 2013, 4:04 p.m. CEST, Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated Sept. 3, 2013, 4:04 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Partly revert commit b03e81a61311ae1b64b0d37415477f9c08fe6142 which ported KFileItem to QFile::Permissions.
Quoting David Faure:
"Port back from QFile::Permissions to mode_t given that mode_t is available in
win32-msvc*/qplatformdefs.h (not everywhere, but in the places where the above
commits did a half job at porting from mode_t to QFile::Permissions).
QFile::Permissions is more limited (no suid bits) and having to convert between
mode_t and QFile::Permissions is just extra work."
I am not sure about the following part of KFileItemPrivate::init():
} else { // link pointing to nowhere (see kio/file/file.cc)
mode = (QT_STAT_MASK - 1) | S_IRWXU | S_IRWXG | S_IRWXO;
}
I blindly brought back from the old code without understanding it. If anything, the comment needs to be updated as there is no kio/file/file.cc anymore.
I am also wondering if this part of the code in isReadable() could be simplified a bit:
// No read permission at all
if (!(S_IRUSR & d->m_permissions) && !(S_IRGRP & d->m_permissions) && !(S_IROTH & d->m_permissions))
return false;
// Read permissions for all: save a stat call
if ((S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions)
return true;
Isn't this the same as this?
return (S_IRUSR|S_IRGRP|S_IROTH) & d->m_permissions;
Same thing for isWritable().</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">All tests pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/tests/kfileitemtest.cpp <span style="color: grey">(c8b7a5d)</span></li>
<li>staging/kio/src/core/kfileitem.h <span style="color: grey">(623b426)</span></li>
<li>staging/kio/src/core/kfileitem.cpp <span style="color: grey">(970dea8)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112481/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>