<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121586/">https://git.reviewboard.kde.org/r/121586/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 18th, 2014, 2:09 a.m. CET, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This breaks the integration with libkomparediff2, doesn't it?</p></pre>
</blockquote>
<p>On December 18th, 2014, 7:47 a.m. CET, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Apart from the hunk count issue I mentioned above, I haven't seen anything that's off - at least not when compared with kdevelop kf5. <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Maybe</em> that changes are no longer highlighted in the review plugin, now that you mention it (no time to go check right now).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What exactly is libkomparediff2 used for, if not for creating the patchfile itself?</p></pre>
</blockquote>
<p>On December 18th, 2014, 1:03 p.m. CET, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, as you can see in the patch you are submitting, the patch is generated by the vcs plugin.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">komparediff2 is used to analyze the generated patch and provide the highlighting.</p></pre>
</blockquote>
<p>On December 18th, 2014, 2:30 p.m. CET, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And the hunk count as well?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is it also komparediff2 that extracts the file paths from the patch? I've tried to follow execution flow, but with the inheritance going on it's very hard to figure out where it goes if not to the base classes...</p></pre>
</blockquote>
<p>On December 18th, 2014, 2:44 p.m. CET, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, hunk count as well. KDevelop itself doesn't do any of the patch analysis.</p></pre>
</blockquote>
<p>On December 18th, 2014, 2:46 p.m. CET, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, then that explains why there isn't a hunk count nor highlighting in KDevelop KF5 either ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This may be naive, but</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> m_kompareInfo<span style="color: #666666">-></span>localSource <span style="color: #666666">=</span> m_patch<span style="color: #666666">-></span>baseDir().toLocalFile();
m_kompareInfo<span style="color: #666666">-></span>depth <span style="color: #666666">=</span> m_patch<span style="color: #666666">-></span>depth();
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Shouldn't these be updated to something like</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> m_kompareInfo<span style="color: #666666">-></span>localSource <span style="color: #666666">=</span> m_patch<span style="color: #666666">-></span>baseDir().upURL().toLocalFile();
m_kompareInfo<span style="color: #666666">-></span>depth <span style="color: #666666">=</span> m_patch<span style="color: #666666">-></span>depth() <span style="color: #666666">+</span> <span style="color: #666666">1</span>;
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">?</p></pre>
</blockquote>
<p>On December 18th, 2014, 3:27 p.m. CET, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Answer: indeed that restores the hunk count and highlighting.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The question now becomes: is this the right place to make that modification, or if not, of what class is m_patch an instance when doing a git patch review? I <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">think</em> I deduced it was a VCSDiffPatchSource, but how to be sure?</p></pre>
</blockquote>
<p>On December 18th, 2014, 3:51 p.m. CET, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the end, we want komparediff2 to support p1 patches.
As a big workaround, you can add the prefix when uploading to reviewboard, if it's a p0 patch, but I think this is very ugly.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, that's not the approach I'd like to take. As shown above, komparediff2 doesn't need to be changed, what needs to be changed is the parameters it receives. It needs to know the depth at which to compare (m_kompareInfo->depth=1 instead of 0), and it needs to receive the parent dir of m_patch->baseDir().</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm currently fighting with the depth parameter, which requires adding a depth instance to VcsDiff, setting that to 1 in gitplugin.cpp and then ensuring that value arrives in vcsdiffpatchsources.cpp .</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On December 17th, 2014, 11:25 p.m. CET, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 17, 2014, 11:25 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A change was made on git.reviewboard.kde.org on December 13th a bit before 22h Europe/Paris time, ending support for p0 patch files. This broke the upload functionality in the current/stable version KDevelop's patchreview plugin.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch under review is a workaround (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">*</code>) that restores this functionality by invoking the git helper process which generates the patchfiles with the correct arguments.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">*</code>) The real bug is in the reviewboard software, and should be addressed at that level. I work with a software distribution system that requires p0 patches, for which I created patches with KDevelop, something which will now require an additional step.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Under OS X 10.9; this RR has been created from a patched copy of KDevelop 4.7 .</p></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>plugins/git/gitplugin.cpp <span style="color: grey">(f38dc71)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121586/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>