<table><tr><td style="">meven 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/D26407">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/D26407#inline-150946">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kmountpoint.cpp:438</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Hmm, what if the symlink *is* the very last component, like your previous iteration tried to handle?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I made the incorrect assumption, I had already checked it</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/D26407#inline-150945">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kmountpoint.cpp:445</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I don't want to be a pain, but this is still wrong....</p>

<p style="padding: 0; margin: 8px;">If /home/dfaure is a symlink to /opt/dfaure, and then /opt/dfaure/tmp is a symlink to /tmp, then the canonical path (and therefore the mount point) for /home/dfaure/tmp is in fact /tmp.</p>

<p style="padding: 0; margin: 8px;">But this is going to call findByPath(/opt/dfaure) (the symlink target of the first symlink found in the path), and stop there, assuming that everything at that target is part of the same mountpoint, which isn't necessarily the case.</p>

<p style="padding: 0; margin: 8px;">I guess this should be findByPath(symLinkTarget() + remainder_of_path)</p>

<p style="padding: 0; margin: 8px;">One possible objection is a case like /a/b/c/d/e/f/g where g is a symlink to h (in the same directory), because then both levels of recursion will stat a, b, c, d, e, f. But maybe this is unavoidable. I don't know how clever canonicalPath() implementations are to optimize such things, while still allowing for /a/b/c/d/e to be a symlink to something totally different, like /x/y, where /x itself might be a symlink (!!).<br />
OK so maybe the recursion and redoing the stat's for all levels is correct.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">On the contrary, thanks this is again a great review.<br />
Sorry for missing cases.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">OK so maybe the recursion and redoing the stat's for all levels is correct.</p></blockquote>

<p style="padding: 0; margin: 8px;">I think so, that's what canonicalFilePath does anyway.</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/D26407">https://phabricator.kde.org/D26407</a></div></div><br /><div><strong>To: </strong>meven, Frameworks, ngraham, broulik, dfaure<br /><strong>Cc: </strong>anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>