<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="https://git.reviewboard.kde.org/r/117044/">https://git.reviewboard.kde.org/r/117044/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 19th, 2014, 12:05 p.m. UTC, <b>Frank Reininghaus</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;">We are seeing quite a few bug reports about a severe regression between KDE SC 4.13.0 and KDE SC 4.13.1:
https://bugs.kde.org/show_bug.cgi?id=334776
According to the reporter of https://bugs.kde.org/show_bug.cgi?id=334988 the regression has been caused by this commit.
I'm surprised that this was committed to KDE/4.13 at all - the review request was for 'master', and it does not really fix a serious bug. Please make sure that such patches, which fix little annoyances but have the potential to cause serious trouble, get a lot of testing in betas and RCs before they are shipped. Testing on a single system is definitely not enough!
I propose to revert this patch - maybe a better solution which prevents automounting can be found for master/frameworks, but IMHO we should not take any further risks in KDE/4.13. Any objections?</pre>
</blockquote>
<p>On May 19th, 2014, 1:50 p.m. UTC, <b>Thomas Lübking</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;">The patch does somehow not what it promises.
KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath( path );
"KMountPoint::currentMountPoints()" promises to return mtab, ie. already used mountpoints, so if there's "mp", it's mounted, iow. it looks like (i've just looked at this for the first time) as if it only shortcuts for already mounted autofs mounts, but will still statvfs on unmounted autofs mounts (mounting them implicitly)
Ie. what the patch probably should do was to:
if (!mp) { // unmounted, avoid automounts
KMountPoint::Ptr pmp = KMountPoint::possibleMountPoints().findByPath(path);
if (pmp && pmp->mountType() == QLatin1String("autofs"))
return info;
}</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;">I think the submitter took David's suggestion and changed the original implementation without understanding the consequences. Thomas' suggested patch would probably be a better starting point.
David: I could not find a isDirectoryMounted() method in kfileitem.cpp to see what you meant by looking at that implementation.
In the meantime I think this patch should be reverted because it caused a regression.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On May 6th, 2014, 8:01 p.m. UTC, Tomáš Trnka wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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 kdelibs.</div>
<div>By Tomáš Trnka.</div>
<p style="color: grey;"><i>Updated May 6, 2014, 8:01 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<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;">Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
Previously, all unmounted autofs mountpoints were always mounted
by krunner/plasma-desktop on startup, defeating the purpose of
automounting. Let's ignore the unmounted filesystems instead when
gathering free space stats, just like the "df" utility does.
Everything still gets mounted properly on first real access.
The change itself is pretty trivial and I would regard it as a bugfix
(and thus eligible for 4.13), but I'm posting it for review to see
what you kdelibs people think.</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;">I'm using this patch on kdelibs since 4.11 and I have noted no problems
in connection with ~4 automounted filesystems.</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/kfile/kdiskfreespaceinfo.cpp <span style="color: grey">(f11eb0998f0e718e9b366f8c26da30586bfa44ef)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117044/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>