<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/106945/">http://git.reviewboard.kde.org/r/106945/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While it's cool that you looked into this, I'm afraid I have to reject the patch in its current form. The code is pretty fragile and this just shows in apparently simple changes like this one which might introduce subtle bugs...
I'll maybe look into this in more depth this week or so and fix it properly.</pre>
<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/106945/diff/2/?file=92822#file92822line82" style="color: black; font-weight: bold; text-decoration: underline;">language/duchain/repositories/itemrepository.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; ">QMutex& ItemRepositoryRegistry::mutex() {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">82</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_ASSERT</span><span class="p">(</span> <span class="n">ICore</span><span class="o">::</span><span class="n">self</span><span class="p">()</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">72</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">repoPath</span> <span class="o">=</span> <span class="n">ItemRepositoryRegistry</span><span class="o">::</span><span class="n">repositoryPathForSession</span><span class="p">(</span> <span class="n">ICore</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">activeSession</span><span class="p">()</span><span class="o">-></span><span class="n">id</span><span class="p">()</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 did you remove the asserts?</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/106945/diff/2/?file=92822#file92822line89" style="color: black; font-weight: bold; text-decoration: underline;">language/duchain/repositories/itemrepository.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; ">QMutex& ItemRepositoryRegistry::mutex() {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">89</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">specific<span class="hl">Dir</span></span> <span class="o">=</span> <span class="n"><span class="hl">baseDir</span></span> <span class="o">+</span> <span class="n">QString</span><span class="p">(</span><span class="s">"/%1"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">a</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">76</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">specificPath</span> <span class="o">=</span> <span class="n">repoPath</span> <span class="o">+</span> <span class="n">QString</span><span class="p">(</span><span class="s">"/%1"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">a</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;">this unrelated cleanup should go into a separate commit and review.
Also, what exactly do you try to achieve by refactoring the code? Personally I think the whole mess should be refactored quite more drastically, considering that a session is required. Since the session has its own locking mechanism (which is much better than what we have here) I think we should just assume that when we have a session uuid, we can blindly allocate the DUchain cache for that uuid.
This will require a change to the cache path though as it does not contain the app name so far.
If you *really* want to work on this, then fine but in a separate commit.</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/106945/diff/2/?file=92823#file92823line122" style="color: black; font-weight: bold; text-decoration: underline;">shell/session.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; ">QUuid Session::id() const</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">122</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">removeDirectory</span><span class="p">(</span> <span class="n">ItemRepositoryRegistry</span><span class="o">::</span><span class="n">repositoryPathForSession</span><span class="p">(</span> <span class="n">d</span><span class="o">-></span><span class="n">info</span><span class="p">.</span><span class="n">uuid</span> <span class="p">)</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;">this is broken, as the DUChain is still "alive" at this point in case of a temporary session.
what we need is a delete function in the DUChain, which either "marks" the DUChain for deletion when we enter aboutToQuit, or directly removes the folder if possible. Then again, the latter requires proper API to ensure we do not delete a running session.</pre>
</div>
<br />
<p>- Milian</p>
<br />
<p>On October 29th, 2012, 2:19 p.m., Ivan Shapovalov 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 KDevelop and Milian Wolff.</div>
<div>By Ivan Shapovalov.</div>
<p style="color: grey;"><i>Updated Oct. 29, 2012, 2:19 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;">1. Clean (rmdir) the per-session duchain store on session removal (particularly useful in unit-tests and their temporary sessions).
2. Partially refactor allocateRepository() (separate cache path compution into a different function, fix formatting, handle lockfiles properly)
Don't know if I've done that properly, but I wanted to avoid mentioning ".cache/kdevduchain" in two unrelated places.</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;">Existing unit-tests and manual testing.</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>language/duchain/repositories/itemrepository.h <span style="color: grey">(392847c)</span></li>
<li>language/duchain/repositories/itemrepository.cpp <span style="color: grey">(8d559cf)</span></li>
<li>shell/session.cpp <span style="color: grey">(a59d70e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106945/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>