<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/106086/">http://git.reviewboard.kde.org/r/106086/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106086/diff/2/?file=81961#file81961line157" style="color: black; font-weight: bold; text-decoration: underline;">services/filewatch/kinotify.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">154</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">addWatchesRecursively</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span><span class="o">&</span> <span class="n">path</span><span class="p">,</span> <span class="kt">int</span><span class="o">&</span> <span class="n">count</span><span class="p">){</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why wouldn't we want to walk the directory tree in subfolders we aren't indexing?
They could have tags/rating and other metadata which we need to preserve on file move events, and delete on a file delete event.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106086/diff/2/?file=81962#file81962line107" style="color: black; font-weight: bold; text-decoration: underline;">services/filewatch/nepomukfilewatch.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">107</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QStringList</span> <span class="n">pathComponents</span> <span class="o">=</span> <span class="n">path</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span class="sc">'/'</span><span class="p">,</span> <span class="n">QString</span><span class="o">::</span><span class="n">SkipEmptyParts</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm a little confused about this.
Shouldn't the shouldFolderBeIndexed on line 100 be enough? Why do we have additional checks to see if the file should be indexed? Specially since the relevant code is there in the shouldFolderByIndexed method.
Also, returning false would make us not add any more watches for any of the subdirectories, which would result in problems. Cause we *always* require the watches, even when there is no indexed metadata. (Tags & Ratings)</pre>
</div>
<br />
<p>- Vishesh</p>
<br />
<p>On August 28th, 2012, 5:42 a.m., Simeon Bird wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated Aug. 28, 2012, 5:42 a.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;">Current master nepomukfilewatcher installs watches on all sub-folders of a watched folder.
This is problematic:
- It means we have to walk the entire directory tree, even for not-indexed folders.
This is quite a lot of work if you happen to have a large complex directory structure
mounted over a network in your $HOME (as I do)
- It means we get inotify watches for directories which are on the filter list; eg, on this computer
$HOME/build/nepomuk-core/services/filewatch/CMakeFiles/nepomukfilewatch.dir/__/fileindexer
is watched, causing the filewatcher to go nuts every time I build something.
- It means we install many more watches than we need to, vastly increasing the probability
of hitting the inotify limit.
This code instead walks the tree until it finds a folder we don't want to index and then STOPS.
I couldn't find a way to avoid walking the whole tree with QDirIterator and QDir::Subdirectories,
so I use QDirIterator without subdirectories, then create a new QDirIterator for each subfolder to index.
I can see two objections to this change:
1) If someone moves a file into an ignored directory, they will now presumably lose their metadata.
This is true, in my opinion not a big problem; the default configuration is to watch
$HOME minus temporary build directories. If people are moving files into temporary
directories they should probably lose the metadata, and if people manually add directories
to the ignore list they probably have a good reason and expect nepomuk to ignore them.
2) I changed filterWatch from always returning true to returning true if we want to watch the
file and false otherwise. I couldn't work out the reason for it always returning true before,
so whatever it was, I've probably broken it.
Bonus fixes:
- Properly pass the return value of addWatch up the tree, so that if we run out of watches,
we stop trying to add more.
- Check for inotify on kernels that have a two-number version string, like 3.0
- To find the length of event->name, qstrlen was used. If an event is returned
for a file outside a watched directory, event->name will not be allocated, and qstrlen
may read beyond the end of allocated memory, causing chaos, anarchy and confusion.
Use qstrnlen instead.
Thanks, let me know what you 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;">Compiled, run, used for a couple of days, checked which files were actually watched, timed the filewatch service's startup.</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>services/filewatch/kinotify.h <span style="color: grey">(ab12d66)</span></li>
<li>services/filewatch/kinotify.cpp <span style="color: grey">(4a744d4)</span></li>
<li>services/filewatch/nepomukfilewatch.cpp <span style="color: grey">(9fd5d9c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106086/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>